* [PATCH v3 1/3] drm/msm/dpu: re-introduce dpu core revision to the catalog
@ 2023-06-29 19:29 ` Abhinav Kumar
0 siblings, 0 replies; 26+ messages in thread
From: Abhinav Kumar @ 2023-06-29 19:29 UTC (permalink / raw)
To: freedreno, Rob Clark, Abhinav Kumar, Dmitry Baryshkov, Sean Paul,
Marijn Suijten, David Airlie, Daniel Vetter
Cc: quic_jesszhan, andersson, linux-kernel, dri-devel, linux-arm-msm
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;
Since dpu's major and minor versions are now separate fields, lets
drop all the DPU_HW_VER macros.
[1]: https://patchwork.freedesktop.org/patch/530891/?series=113910&rev=4
changes in v3:
- drop DPU step version as features are not changing across steps
- add core_major_version / core_minor_version to avoid conflicts
- update the commit text to drop references to the dpu macros
Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h | 2 ++
drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h | 2 ++
drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h | 2 ++
drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h | 2 ++
drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h | 2 ++
drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h | 2 ++
drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h | 2 ++
drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_4_sm6350.h | 2 ++
drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h | 2 ++
drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_9_sm6375.h | 2 ++
drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h | 2 ++
drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h | 2 ++
drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h | 2 ++
drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h | 2 ++
drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h | 2 ++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 8 ++++++--
16 files changed, 36 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
index 7d0d0e74c3b0..a5d486783c3f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
@@ -190,6 +190,8 @@ static const struct dpu_perf_cfg msm8998_perf_data = {
};
const struct dpu_mdss_cfg dpu_msm8998_cfg = {
+ .core_major_version = 0x3,
+ .core_minor_version = 0x0,
.caps = &msm8998_dpu_caps,
.ubwc = &msm8998_ubwc_cfg,
.mdp_count = ARRAY_SIZE(msm8998_mdp),
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h
index b6098141bb9b..1fdb89a4b7a6 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h
@@ -194,6 +194,8 @@ static const struct dpu_perf_cfg sdm845_perf_data = {
};
const struct dpu_mdss_cfg dpu_sdm845_cfg = {
+ .core_major_version = 0x4,
+ .core_minor_version = 0x0,
.caps = &sdm845_dpu_caps,
.ubwc = &sdm845_ubwc_cfg,
.mdp_count = ARRAY_SIZE(sdm845_mdp),
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h
index b5f751354267..129c62cf450d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h
@@ -208,6 +208,8 @@ static const struct dpu_perf_cfg sm8150_perf_data = {
};
const struct dpu_mdss_cfg dpu_sm8150_cfg = {
+ .core_major_version = 0x5,
+ .core_minor_version = 0x0,
.caps = &sm8150_dpu_caps,
.ubwc = &sm8150_ubwc_cfg,
.mdp_count = ARRAY_SIZE(sm8150_mdp),
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h
index 8ed2b263c5ea..ca037b070f44 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h
@@ -214,6 +214,8 @@ static const struct dpu_perf_cfg sc8180x_perf_data = {
};
const struct dpu_mdss_cfg dpu_sc8180x_cfg = {
+ .core_major_version = 0x5,
+ .core_minor_version = 0x1,
.caps = &sc8180x_dpu_caps,
.ubwc = &sc8180x_ubwc_cfg,
.mdp_count = ARRAY_SIZE(sc8180x_mdp),
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h
index daebd2170041..e446af90767e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h
@@ -214,6 +214,8 @@ static const struct dpu_perf_cfg sm8250_perf_data = {
};
const struct dpu_mdss_cfg dpu_sm8250_cfg = {
+ .core_major_version = 0x6,
+ .core_minor_version = 0x0,
.caps = &sm8250_dpu_caps,
.ubwc = &sm8250_ubwc_cfg,
.mdp_count = ARRAY_SIZE(sm8250_mdp),
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h
index 67566b07195a..88288c80b652 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h
@@ -132,6 +132,8 @@ static const struct dpu_perf_cfg sc7180_perf_data = {
};
const struct dpu_mdss_cfg dpu_sc7180_cfg = {
+ .core_major_version = 0x6,
+ .core_minor_version = 0x2,
.caps = &sc7180_dpu_caps,
.ubwc = &sc7180_ubwc_cfg,
.mdp_count = ARRAY_SIZE(sc7180_mdp),
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h
index 031fc8dae3c6..93c901502b5a 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h
@@ -102,6 +102,8 @@ static const struct dpu_perf_cfg sm6115_perf_data = {
};
const struct dpu_mdss_cfg dpu_sm6115_cfg = {
+ .core_major_version = 0x6,
+ .core_minor_version = 0x3,
.caps = &sm6115_dpu_caps,
.ubwc = &sm6115_ubwc_cfg,
.mdp_count = ARRAY_SIZE(sm6115_mdp),
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_4_sm6350.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_4_sm6350.h
index 06eba23b0236..ff7e4b775fd5 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_4_sm6350.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_4_sm6350.h
@@ -141,6 +141,8 @@ static const struct dpu_perf_cfg sm6350_perf_data = {
};
const struct dpu_mdss_cfg dpu_sm6350_cfg = {
+ .core_major_version = 0x6,
+ .core_minor_version = 0x4,
.caps = &sm6350_dpu_caps,
.ubwc = &sm6350_ubwc_cfg,
.mdp_count = ARRAY_SIZE(sm6350_mdp),
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h
index f2808098af39..7bc86aa50e6f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h
@@ -92,6 +92,8 @@ static const struct dpu_perf_cfg qcm2290_perf_data = {
};
const struct dpu_mdss_cfg dpu_qcm2290_cfg = {
+ .core_major_version = 0x6,
+ .core_minor_version = 0x5,
.caps = &qcm2290_dpu_caps,
.ubwc = &qcm2290_ubwc_cfg,
.mdp_count = ARRAY_SIZE(qcm2290_mdp),
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_9_sm6375.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_9_sm6375.h
index 241fa6746674..d92890f090d4 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_9_sm6375.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_9_sm6375.h
@@ -107,6 +107,8 @@ static const struct dpu_perf_cfg sm6375_perf_data = {
};
const struct dpu_mdss_cfg dpu_sm6375_cfg = {
+ .core_major_version = 0x6,
+ .core_minor_version = 0x9,
.caps = &sm6375_dpu_caps,
.ubwc = &sm6375_ubwc_cfg,
.mdp_count = ARRAY_SIZE(sm6375_mdp),
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
index 8da424eaee6a..8a2dc56c79f8 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
@@ -213,6 +213,8 @@ static const struct dpu_perf_cfg sm8350_perf_data = {
};
const struct dpu_mdss_cfg dpu_sm8350_cfg = {
+ .core_major_version = 0x7,
+ .core_minor_version = 0x0,
.caps = &sm8350_dpu_caps,
.ubwc = &sm8350_ubwc_cfg,
.mdp_count = ARRAY_SIZE(sm8350_mdp),
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
index 900fee410e11..bba7bdb9bd42 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
@@ -154,6 +154,8 @@ static const struct dpu_perf_cfg sc7280_perf_data = {
};
const struct dpu_mdss_cfg dpu_sc7280_cfg = {
+ .core_major_version = 0x7,
+ .core_minor_version = 0x2,
.caps = &sc7280_dpu_caps,
.ubwc = &sc7280_ubwc_cfg,
.mdp_count = ARRAY_SIZE(sc7280_mdp),
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
index f6ce6b090f71..3f51b802b6b7 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
@@ -217,6 +217,8 @@ static const struct dpu_perf_cfg sc8280xp_perf_data = {
};
const struct dpu_mdss_cfg dpu_sc8280xp_cfg = {
+ .core_major_version = 0x8,
+ .core_minor_version = 0x0,
.caps = &sc8280xp_dpu_caps,
.ubwc = &sc8280xp_ubwc_cfg,
.mdp_count = ARRAY_SIZE(sc8280xp_mdp),
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
index 8d13c369213c..20acff9db979 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
@@ -221,6 +221,8 @@ static const struct dpu_perf_cfg sm8450_perf_data = {
};
const struct dpu_mdss_cfg dpu_sm8450_cfg = {
+ .core_major_version = 0x8,
+ .core_minor_version = 0x1,
.caps = &sm8450_dpu_caps,
.ubwc = &sm8450_ubwc_cfg,
.mdp_count = ARRAY_SIZE(sm8450_mdp),
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
index f17b9a7fee85..89fdf334a0aa 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
@@ -225,6 +225,8 @@ static const struct dpu_perf_cfg sm8550_perf_data = {
};
const struct dpu_mdss_cfg dpu_sm8550_cfg = {
+ .core_major_version = 0x9,
+ .core_minor_version = 0x0,
.caps = &sm8550_dpu_caps,
.ubwc = &sm8550_ubwc_cfg,
.mdp_count = ARRAY_SIZE(sm8550_mdp),
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 b860784ade72..8b900be3ea90 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
@@ -796,8 +796,9 @@ struct dpu_perf_cfg {
/**
* struct dpu_mdss_cfg - information of MDSS HW
* This is the main catalog data structure representing
- * this HW version. Contains number of instances,
- * register offsets, capabilities of the all MDSS HW sub-blocks.
+ * this HW version. Contains dpu's major and minor versions,
+ * number of instances, register offsets, capabilities of the
+ * all MDSS HW sub-blocks.
*
* @dma_formats Supported formats for dma pipe
* @cursor_formats Supported formats for cursor pipe
@@ -805,6 +806,9 @@ struct dpu_perf_cfg {
* @mdss_irqs: Bitmap with the irqs supported by the target
*/
struct dpu_mdss_cfg {
+ u8 core_major_version;
+ u8 core_minor_version;
+
const struct dpu_caps *caps;
const struct dpu_ubwc_cfg *ubwc;
--
2.40.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v3 1/3] drm/msm/dpu: re-introduce dpu core revision to the catalog
@ 2023-06-29 19:29 ` Abhinav Kumar
0 siblings, 0 replies; 26+ messages in thread
From: Abhinav Kumar @ 2023-06-29 19:29 UTC (permalink / raw)
To: freedreno, Rob Clark, Abhinav Kumar, Dmitry Baryshkov, Sean Paul,
Marijn Suijten, David Airlie, Daniel Vetter
Cc: dri-devel, quic_jesszhan, andersson, linux-arm-msm, linux-kernel
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;
Since dpu's major and minor versions are now separate fields, lets
drop all the DPU_HW_VER macros.
[1]: https://patchwork.freedesktop.org/patch/530891/?series=113910&rev=4
changes in v3:
- drop DPU step version as features are not changing across steps
- add core_major_version / core_minor_version to avoid conflicts
- update the commit text to drop references to the dpu macros
Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h | 2 ++
drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h | 2 ++
drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h | 2 ++
drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h | 2 ++
drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h | 2 ++
drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h | 2 ++
drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h | 2 ++
drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_4_sm6350.h | 2 ++
drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h | 2 ++
drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_9_sm6375.h | 2 ++
drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h | 2 ++
drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h | 2 ++
drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h | 2 ++
drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h | 2 ++
drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h | 2 ++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 8 ++++++--
16 files changed, 36 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
index 7d0d0e74c3b0..a5d486783c3f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
@@ -190,6 +190,8 @@ static const struct dpu_perf_cfg msm8998_perf_data = {
};
const struct dpu_mdss_cfg dpu_msm8998_cfg = {
+ .core_major_version = 0x3,
+ .core_minor_version = 0x0,
.caps = &msm8998_dpu_caps,
.ubwc = &msm8998_ubwc_cfg,
.mdp_count = ARRAY_SIZE(msm8998_mdp),
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h
index b6098141bb9b..1fdb89a4b7a6 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h
@@ -194,6 +194,8 @@ static const struct dpu_perf_cfg sdm845_perf_data = {
};
const struct dpu_mdss_cfg dpu_sdm845_cfg = {
+ .core_major_version = 0x4,
+ .core_minor_version = 0x0,
.caps = &sdm845_dpu_caps,
.ubwc = &sdm845_ubwc_cfg,
.mdp_count = ARRAY_SIZE(sdm845_mdp),
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h
index b5f751354267..129c62cf450d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h
@@ -208,6 +208,8 @@ static const struct dpu_perf_cfg sm8150_perf_data = {
};
const struct dpu_mdss_cfg dpu_sm8150_cfg = {
+ .core_major_version = 0x5,
+ .core_minor_version = 0x0,
.caps = &sm8150_dpu_caps,
.ubwc = &sm8150_ubwc_cfg,
.mdp_count = ARRAY_SIZE(sm8150_mdp),
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h
index 8ed2b263c5ea..ca037b070f44 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h
@@ -214,6 +214,8 @@ static const struct dpu_perf_cfg sc8180x_perf_data = {
};
const struct dpu_mdss_cfg dpu_sc8180x_cfg = {
+ .core_major_version = 0x5,
+ .core_minor_version = 0x1,
.caps = &sc8180x_dpu_caps,
.ubwc = &sc8180x_ubwc_cfg,
.mdp_count = ARRAY_SIZE(sc8180x_mdp),
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h
index daebd2170041..e446af90767e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h
@@ -214,6 +214,8 @@ static const struct dpu_perf_cfg sm8250_perf_data = {
};
const struct dpu_mdss_cfg dpu_sm8250_cfg = {
+ .core_major_version = 0x6,
+ .core_minor_version = 0x0,
.caps = &sm8250_dpu_caps,
.ubwc = &sm8250_ubwc_cfg,
.mdp_count = ARRAY_SIZE(sm8250_mdp),
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h
index 67566b07195a..88288c80b652 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h
@@ -132,6 +132,8 @@ static const struct dpu_perf_cfg sc7180_perf_data = {
};
const struct dpu_mdss_cfg dpu_sc7180_cfg = {
+ .core_major_version = 0x6,
+ .core_minor_version = 0x2,
.caps = &sc7180_dpu_caps,
.ubwc = &sc7180_ubwc_cfg,
.mdp_count = ARRAY_SIZE(sc7180_mdp),
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h
index 031fc8dae3c6..93c901502b5a 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h
@@ -102,6 +102,8 @@ static const struct dpu_perf_cfg sm6115_perf_data = {
};
const struct dpu_mdss_cfg dpu_sm6115_cfg = {
+ .core_major_version = 0x6,
+ .core_minor_version = 0x3,
.caps = &sm6115_dpu_caps,
.ubwc = &sm6115_ubwc_cfg,
.mdp_count = ARRAY_SIZE(sm6115_mdp),
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_4_sm6350.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_4_sm6350.h
index 06eba23b0236..ff7e4b775fd5 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_4_sm6350.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_4_sm6350.h
@@ -141,6 +141,8 @@ static const struct dpu_perf_cfg sm6350_perf_data = {
};
const struct dpu_mdss_cfg dpu_sm6350_cfg = {
+ .core_major_version = 0x6,
+ .core_minor_version = 0x4,
.caps = &sm6350_dpu_caps,
.ubwc = &sm6350_ubwc_cfg,
.mdp_count = ARRAY_SIZE(sm6350_mdp),
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h
index f2808098af39..7bc86aa50e6f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h
@@ -92,6 +92,8 @@ static const struct dpu_perf_cfg qcm2290_perf_data = {
};
const struct dpu_mdss_cfg dpu_qcm2290_cfg = {
+ .core_major_version = 0x6,
+ .core_minor_version = 0x5,
.caps = &qcm2290_dpu_caps,
.ubwc = &qcm2290_ubwc_cfg,
.mdp_count = ARRAY_SIZE(qcm2290_mdp),
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_9_sm6375.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_9_sm6375.h
index 241fa6746674..d92890f090d4 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_9_sm6375.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_9_sm6375.h
@@ -107,6 +107,8 @@ static const struct dpu_perf_cfg sm6375_perf_data = {
};
const struct dpu_mdss_cfg dpu_sm6375_cfg = {
+ .core_major_version = 0x6,
+ .core_minor_version = 0x9,
.caps = &sm6375_dpu_caps,
.ubwc = &sm6375_ubwc_cfg,
.mdp_count = ARRAY_SIZE(sm6375_mdp),
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
index 8da424eaee6a..8a2dc56c79f8 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
@@ -213,6 +213,8 @@ static const struct dpu_perf_cfg sm8350_perf_data = {
};
const struct dpu_mdss_cfg dpu_sm8350_cfg = {
+ .core_major_version = 0x7,
+ .core_minor_version = 0x0,
.caps = &sm8350_dpu_caps,
.ubwc = &sm8350_ubwc_cfg,
.mdp_count = ARRAY_SIZE(sm8350_mdp),
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
index 900fee410e11..bba7bdb9bd42 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
@@ -154,6 +154,8 @@ static const struct dpu_perf_cfg sc7280_perf_data = {
};
const struct dpu_mdss_cfg dpu_sc7280_cfg = {
+ .core_major_version = 0x7,
+ .core_minor_version = 0x2,
.caps = &sc7280_dpu_caps,
.ubwc = &sc7280_ubwc_cfg,
.mdp_count = ARRAY_SIZE(sc7280_mdp),
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
index f6ce6b090f71..3f51b802b6b7 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
@@ -217,6 +217,8 @@ static const struct dpu_perf_cfg sc8280xp_perf_data = {
};
const struct dpu_mdss_cfg dpu_sc8280xp_cfg = {
+ .core_major_version = 0x8,
+ .core_minor_version = 0x0,
.caps = &sc8280xp_dpu_caps,
.ubwc = &sc8280xp_ubwc_cfg,
.mdp_count = ARRAY_SIZE(sc8280xp_mdp),
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
index 8d13c369213c..20acff9db979 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
@@ -221,6 +221,8 @@ static const struct dpu_perf_cfg sm8450_perf_data = {
};
const struct dpu_mdss_cfg dpu_sm8450_cfg = {
+ .core_major_version = 0x8,
+ .core_minor_version = 0x1,
.caps = &sm8450_dpu_caps,
.ubwc = &sm8450_ubwc_cfg,
.mdp_count = ARRAY_SIZE(sm8450_mdp),
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
index f17b9a7fee85..89fdf334a0aa 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
@@ -225,6 +225,8 @@ static const struct dpu_perf_cfg sm8550_perf_data = {
};
const struct dpu_mdss_cfg dpu_sm8550_cfg = {
+ .core_major_version = 0x9,
+ .core_minor_version = 0x0,
.caps = &sm8550_dpu_caps,
.ubwc = &sm8550_ubwc_cfg,
.mdp_count = ARRAY_SIZE(sm8550_mdp),
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 b860784ade72..8b900be3ea90 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
@@ -796,8 +796,9 @@ struct dpu_perf_cfg {
/**
* struct dpu_mdss_cfg - information of MDSS HW
* This is the main catalog data structure representing
- * this HW version. Contains number of instances,
- * register offsets, capabilities of the all MDSS HW sub-blocks.
+ * this HW version. Contains dpu's major and minor versions,
+ * number of instances, register offsets, capabilities of the
+ * all MDSS HW sub-blocks.
*
* @dma_formats Supported formats for dma pipe
* @cursor_formats Supported formats for cursor pipe
@@ -805,6 +806,9 @@ struct dpu_perf_cfg {
* @mdss_irqs: Bitmap with the irqs supported by the target
*/
struct dpu_mdss_cfg {
+ u8 core_major_version;
+ u8 core_minor_version;
+
const struct dpu_caps *caps;
const struct dpu_ubwc_cfg *ubwc;
--
2.40.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v3 2/3] drm/msm/dpu: use dpu core's major version to enable data compress
2023-06-29 19:29 ` Abhinav Kumar
@ 2023-06-29 19:29 ` Abhinav Kumar
-1 siblings, 0 replies; 26+ messages in thread
From: Abhinav Kumar @ 2023-06-29 19:29 UTC (permalink / raw)
To: freedreno, Rob Clark, Abhinav Kumar, Dmitry Baryshkov, Sean Paul,
Marijn Suijten, David Airlie, Daniel Vetter
Cc: quic_jesszhan, andersson, linux-kernel, dri-devel, linux-arm-msm
Instead of using a feature bit to decide whether to enable data
compress or not for DSC use-cases, use dpu core's major version instead.
This will avoid defining feature bits for every bit level details of
registers.
Also, rename the intf's enable_compression() op to program_datapath()
and allow it to accept a struct intf_dpu_datapath_cfg to program
all the bits at once. This can be re-used by widebus later on as
well as it touches the same register.
Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 9 +++++++--
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 9 +++++----
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 16 ++++++++++++++--
3 files changed, 26 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
index b856c6286c85..f4e15b4c4cc9 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
@@ -50,6 +50,7 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
to_dpu_encoder_phys_cmd(phys_enc);
struct dpu_hw_ctl *ctl;
struct dpu_hw_intf_cfg intf_cfg = { 0 };
+ struct dpu_kms *dpu_kms = phys_enc->dpu_kms;
ctl = phys_enc->hw_ctl;
if (!ctl->ops.setup_intf_cfg)
@@ -68,8 +69,12 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
phys_enc->hw_intf,
phys_enc->hw_pp->idx);
- if (intf_cfg.dsc != 0 && phys_enc->hw_intf->ops.enable_compression)
- phys_enc->hw_intf->ops.enable_compression(phys_enc->hw_intf);
+ if (intf_cfg.dsc != 0 && dpu_kms->catalog->core_major_version >= 0x7) {
+ struct intf_dpu_datapath_cfg datapath_cfg = { 0 };
+
+ datapath_cfg.data_compress = true;
+ phys_enc->hw_intf->ops.program_datapath(phys_enc->hw_intf, &datapath_cfg);
+ }
}
static void dpu_encoder_phys_cmd_pp_tx_done_irq(void *arg, int irq_idx)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
index 5b0f6627e29b..85333df08fbc 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
@@ -513,11 +513,13 @@ static void dpu_hw_intf_disable_autorefresh(struct dpu_hw_intf *intf,
}
-static void dpu_hw_intf_enable_compression(struct dpu_hw_intf *ctx)
+static void dpu_hw_intf_program_datapath(struct dpu_hw_intf *ctx,
+ struct intf_dpu_datapath_cfg *datapath_cfg)
{
u32 intf_cfg2 = DPU_REG_READ(&ctx->hw, INTF_CONFIG2);
- intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
+ if (datapath_cfg->data_compress)
+ intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, intf_cfg2);
}
@@ -543,8 +545,7 @@ static void _setup_intf_ops(struct dpu_hw_intf_ops *ops,
ops->disable_autorefresh = dpu_hw_intf_disable_autorefresh;
}
- if (cap & BIT(DPU_INTF_DATA_COMPRESS))
- ops->enable_compression = dpu_hw_intf_enable_compression;
+ ops->program_datapath = dpu_hw_intf_program_datapath;
}
struct dpu_hw_intf *dpu_hw_intf_init(const struct dpu_intf_cfg *cfg,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
index 99e21c4137f9..f736dca38463 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
@@ -48,6 +48,11 @@ struct intf_status {
u32 line_count; /* current line count including blanking */
};
+struct intf_dpu_datapath_cfg {
+ u8 data_compress; /* enable data compress between dpu and dsi */
+ /* can be expanded for other programmable bits */
+};
+
/**
* struct dpu_hw_intf_ops : Interface to the interface Hw driver functions
* Assumption is these functions will be called after clocks are enabled
@@ -70,7 +75,7 @@ struct intf_status {
* @get_autorefresh: Retrieve autorefresh config from hardware
* Return: 0 on success, -ETIMEDOUT on timeout
* @vsync_sel: Select vsync signal for tear-effect configuration
- * @enable_compression: Enable data compression
+ * @program_datapath: Program the DPU to interface datapath for relevant chipsets
*/
struct dpu_hw_intf_ops {
void (*setup_timing_gen)(struct dpu_hw_intf *intf,
@@ -108,7 +113,14 @@ struct dpu_hw_intf_ops {
*/
void (*disable_autorefresh)(struct dpu_hw_intf *intf, uint32_t encoder_id, u16 vdisplay);
- void (*enable_compression)(struct dpu_hw_intf *intf);
+ /**
+ * Program the DPU to intf datapath by specifying
+ * which of the settings need to be programmed for
+ * use-cases which need DPU-intf handshake such as
+ * widebus, compression etc.
+ */
+ void (*program_datapath)(struct dpu_hw_intf *intf,
+ struct intf_dpu_datapath_cfg *datapath_cfg);
};
struct dpu_hw_intf {
--
2.40.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v3 2/3] drm/msm/dpu: use dpu core's major version to enable data compress
@ 2023-06-29 19:29 ` Abhinav Kumar
0 siblings, 0 replies; 26+ messages in thread
From: Abhinav Kumar @ 2023-06-29 19:29 UTC (permalink / raw)
To: freedreno, Rob Clark, Abhinav Kumar, Dmitry Baryshkov, Sean Paul,
Marijn Suijten, David Airlie, Daniel Vetter
Cc: dri-devel, quic_jesszhan, andersson, linux-arm-msm, linux-kernel
Instead of using a feature bit to decide whether to enable data
compress or not for DSC use-cases, use dpu core's major version instead.
This will avoid defining feature bits for every bit level details of
registers.
Also, rename the intf's enable_compression() op to program_datapath()
and allow it to accept a struct intf_dpu_datapath_cfg to program
all the bits at once. This can be re-used by widebus later on as
well as it touches the same register.
Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 9 +++++++--
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 9 +++++----
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 16 ++++++++++++++--
3 files changed, 26 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
index b856c6286c85..f4e15b4c4cc9 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
@@ -50,6 +50,7 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
to_dpu_encoder_phys_cmd(phys_enc);
struct dpu_hw_ctl *ctl;
struct dpu_hw_intf_cfg intf_cfg = { 0 };
+ struct dpu_kms *dpu_kms = phys_enc->dpu_kms;
ctl = phys_enc->hw_ctl;
if (!ctl->ops.setup_intf_cfg)
@@ -68,8 +69,12 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
phys_enc->hw_intf,
phys_enc->hw_pp->idx);
- if (intf_cfg.dsc != 0 && phys_enc->hw_intf->ops.enable_compression)
- phys_enc->hw_intf->ops.enable_compression(phys_enc->hw_intf);
+ if (intf_cfg.dsc != 0 && dpu_kms->catalog->core_major_version >= 0x7) {
+ struct intf_dpu_datapath_cfg datapath_cfg = { 0 };
+
+ datapath_cfg.data_compress = true;
+ phys_enc->hw_intf->ops.program_datapath(phys_enc->hw_intf, &datapath_cfg);
+ }
}
static void dpu_encoder_phys_cmd_pp_tx_done_irq(void *arg, int irq_idx)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
index 5b0f6627e29b..85333df08fbc 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
@@ -513,11 +513,13 @@ static void dpu_hw_intf_disable_autorefresh(struct dpu_hw_intf *intf,
}
-static void dpu_hw_intf_enable_compression(struct dpu_hw_intf *ctx)
+static void dpu_hw_intf_program_datapath(struct dpu_hw_intf *ctx,
+ struct intf_dpu_datapath_cfg *datapath_cfg)
{
u32 intf_cfg2 = DPU_REG_READ(&ctx->hw, INTF_CONFIG2);
- intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
+ if (datapath_cfg->data_compress)
+ intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, intf_cfg2);
}
@@ -543,8 +545,7 @@ static void _setup_intf_ops(struct dpu_hw_intf_ops *ops,
ops->disable_autorefresh = dpu_hw_intf_disable_autorefresh;
}
- if (cap & BIT(DPU_INTF_DATA_COMPRESS))
- ops->enable_compression = dpu_hw_intf_enable_compression;
+ ops->program_datapath = dpu_hw_intf_program_datapath;
}
struct dpu_hw_intf *dpu_hw_intf_init(const struct dpu_intf_cfg *cfg,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
index 99e21c4137f9..f736dca38463 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
@@ -48,6 +48,11 @@ struct intf_status {
u32 line_count; /* current line count including blanking */
};
+struct intf_dpu_datapath_cfg {
+ u8 data_compress; /* enable data compress between dpu and dsi */
+ /* can be expanded for other programmable bits */
+};
+
/**
* struct dpu_hw_intf_ops : Interface to the interface Hw driver functions
* Assumption is these functions will be called after clocks are enabled
@@ -70,7 +75,7 @@ struct intf_status {
* @get_autorefresh: Retrieve autorefresh config from hardware
* Return: 0 on success, -ETIMEDOUT on timeout
* @vsync_sel: Select vsync signal for tear-effect configuration
- * @enable_compression: Enable data compression
+ * @program_datapath: Program the DPU to interface datapath for relevant chipsets
*/
struct dpu_hw_intf_ops {
void (*setup_timing_gen)(struct dpu_hw_intf *intf,
@@ -108,7 +113,14 @@ struct dpu_hw_intf_ops {
*/
void (*disable_autorefresh)(struct dpu_hw_intf *intf, uint32_t encoder_id, u16 vdisplay);
- void (*enable_compression)(struct dpu_hw_intf *intf);
+ /**
+ * Program the DPU to intf datapath by specifying
+ * which of the settings need to be programmed for
+ * use-cases which need DPU-intf handshake such as
+ * widebus, compression etc.
+ */
+ void (*program_datapath)(struct dpu_hw_intf *intf,
+ struct intf_dpu_datapath_cfg *datapath_cfg);
};
struct dpu_hw_intf {
--
2.40.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v3 3/3] drm/msm/dpu: drop DPU_INTF_DATA_COMPRESS from dpu catalog
2023-06-29 19:29 ` Abhinav Kumar
@ 2023-06-29 19:29 ` Abhinav Kumar
-1 siblings, 0 replies; 26+ messages in thread
From: Abhinav Kumar @ 2023-06-29 19:29 UTC (permalink / raw)
To: freedreno, Rob Clark, Abhinav Kumar, Dmitry Baryshkov, Sean Paul,
Marijn Suijten, David Airlie, Daniel Vetter
Cc: quic_jesszhan, andersson, linux-kernel, dri-devel, linux-arm-msm
Now that all usages of DPU_INTF_DATA_COMPRESS have been replaced
with the dpu core's major revision lets drop DPU_INTF_DATA_COMPRESS
from the catalog completely.
Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 2 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 --
2 files changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
index 0de507d4d7b7..35994e676450 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -105,7 +105,7 @@
BIT(DPU_INTF_STATUS_SUPPORTED) | \
BIT(DPU_DATA_HCTL_EN))
-#define INTF_SC7280_MASK (INTF_SC7180_MASK | BIT(DPU_INTF_DATA_COMPRESS))
+#define INTF_SC7280_MASK (INTF_SC7180_MASK)
#define WB_SM8250_MASK (BIT(DPU_WB_LINE_MODE) | \
BIT(DPU_WB_UBWC) | \
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 8b900be3ea90..572e618150b8 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
@@ -181,7 +181,6 @@ enum {
* @DPU_DATA_HCTL_EN Allows data to be transferred at different rate
* than video timing
* @DPU_INTF_STATUS_SUPPORTED INTF block has INTF_STATUS register
- * @DPU_INTF_DATA_COMPRESS INTF block has DATA_COMPRESS register
* @DPU_INTF_MAX
*/
enum {
@@ -189,7 +188,6 @@ enum {
DPU_INTF_TE,
DPU_DATA_HCTL_EN,
DPU_INTF_STATUS_SUPPORTED,
- DPU_INTF_DATA_COMPRESS,
DPU_INTF_MAX
};
--
2.40.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v3 3/3] drm/msm/dpu: drop DPU_INTF_DATA_COMPRESS from dpu catalog
@ 2023-06-29 19:29 ` Abhinav Kumar
0 siblings, 0 replies; 26+ messages in thread
From: Abhinav Kumar @ 2023-06-29 19:29 UTC (permalink / raw)
To: freedreno, Rob Clark, Abhinav Kumar, Dmitry Baryshkov, Sean Paul,
Marijn Suijten, David Airlie, Daniel Vetter
Cc: dri-devel, quic_jesszhan, andersson, linux-arm-msm, linux-kernel
Now that all usages of DPU_INTF_DATA_COMPRESS have been replaced
with the dpu core's major revision lets drop DPU_INTF_DATA_COMPRESS
from the catalog completely.
Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 2 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 --
2 files changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
index 0de507d4d7b7..35994e676450 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -105,7 +105,7 @@
BIT(DPU_INTF_STATUS_SUPPORTED) | \
BIT(DPU_DATA_HCTL_EN))
-#define INTF_SC7280_MASK (INTF_SC7180_MASK | BIT(DPU_INTF_DATA_COMPRESS))
+#define INTF_SC7280_MASK (INTF_SC7180_MASK)
#define WB_SM8250_MASK (BIT(DPU_WB_LINE_MODE) | \
BIT(DPU_WB_UBWC) | \
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 8b900be3ea90..572e618150b8 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
@@ -181,7 +181,6 @@ enum {
* @DPU_DATA_HCTL_EN Allows data to be transferred at different rate
* than video timing
* @DPU_INTF_STATUS_SUPPORTED INTF block has INTF_STATUS register
- * @DPU_INTF_DATA_COMPRESS INTF block has DATA_COMPRESS register
* @DPU_INTF_MAX
*/
enum {
@@ -189,7 +188,6 @@ enum {
DPU_INTF_TE,
DPU_DATA_HCTL_EN,
DPU_INTF_STATUS_SUPPORTED,
- DPU_INTF_DATA_COMPRESS,
DPU_INTF_MAX
};
--
2.40.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v3 1/3] drm/msm/dpu: re-introduce dpu core revision to the catalog
2023-06-29 19:29 ` Abhinav Kumar
@ 2023-06-30 0:13 ` Dmitry Baryshkov
-1 siblings, 0 replies; 26+ messages in thread
From: Dmitry Baryshkov @ 2023-06-30 0:13 UTC (permalink / raw)
To: Abhinav Kumar, freedreno, Rob Clark, Sean Paul, Marijn Suijten,
David Airlie, Daniel Vetter
Cc: dri-devel, quic_jesszhan, andersson, linux-arm-msm, linux-kernel
On 29/06/2023 22:29, Abhinav Kumar 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;
>
> Since dpu's major and minor versions are now separate fields, lets
> drop all the DPU_HW_VER macros.
>
> [1]: https://patchwork.freedesktop.org/patch/530891/?series=113910&rev=4
>
> changes in v3:
> - drop DPU step version as features are not changing across steps
> - add core_major_version / core_minor_version to avoid conflicts
> - update the commit text to drop references to the dpu macros
>
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h | 2 ++
> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h | 2 ++
> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h | 2 ++
> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h | 2 ++
> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h | 2 ++
> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h | 2 ++
> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h | 2 ++
> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_4_sm6350.h | 2 ++
> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h | 2 ++
> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_9_sm6375.h | 2 ++
> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h | 2 ++
> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h | 2 ++
> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h | 2 ++
> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h | 2 ++
> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h | 2 ++
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 8 ++++++--
> 16 files changed, 36 insertions(+), 2 deletions(-)
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Nit: if there is a need for v-next, I'd probably suggest dropping th hex
prefix from these values.
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
> index 7d0d0e74c3b0..a5d486783c3f 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
> @@ -190,6 +190,8 @@ static const struct dpu_perf_cfg msm8998_perf_data = {
> };
>
> const struct dpu_mdss_cfg dpu_msm8998_cfg = {
> + .core_major_version = 0x3,
> + .core_minor_version = 0x0,
> .caps = &msm8998_dpu_caps,
> .ubwc = &msm8998_ubwc_cfg,
> .mdp_count = ARRAY_SIZE(msm8998_mdp),
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h
> index b6098141bb9b..1fdb89a4b7a6 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h
> @@ -194,6 +194,8 @@ static const struct dpu_perf_cfg sdm845_perf_data = {
> };
>
> const struct dpu_mdss_cfg dpu_sdm845_cfg = {
> + .core_major_version = 0x4,
> + .core_minor_version = 0x0,
> .caps = &sdm845_dpu_caps,
> .ubwc = &sdm845_ubwc_cfg,
> .mdp_count = ARRAY_SIZE(sdm845_mdp),
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h
> index b5f751354267..129c62cf450d 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h
> @@ -208,6 +208,8 @@ static const struct dpu_perf_cfg sm8150_perf_data = {
> };
>
> const struct dpu_mdss_cfg dpu_sm8150_cfg = {
> + .core_major_version = 0x5,
> + .core_minor_version = 0x0,
> .caps = &sm8150_dpu_caps,
> .ubwc = &sm8150_ubwc_cfg,
> .mdp_count = ARRAY_SIZE(sm8150_mdp),
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h
> index 8ed2b263c5ea..ca037b070f44 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h
> @@ -214,6 +214,8 @@ static const struct dpu_perf_cfg sc8180x_perf_data = {
> };
>
> const struct dpu_mdss_cfg dpu_sc8180x_cfg = {
> + .core_major_version = 0x5,
> + .core_minor_version = 0x1,
> .caps = &sc8180x_dpu_caps,
> .ubwc = &sc8180x_ubwc_cfg,
> .mdp_count = ARRAY_SIZE(sc8180x_mdp),
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h
> index daebd2170041..e446af90767e 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h
> @@ -214,6 +214,8 @@ static const struct dpu_perf_cfg sm8250_perf_data = {
> };
>
> const struct dpu_mdss_cfg dpu_sm8250_cfg = {
> + .core_major_version = 0x6,
> + .core_minor_version = 0x0,
> .caps = &sm8250_dpu_caps,
> .ubwc = &sm8250_ubwc_cfg,
> .mdp_count = ARRAY_SIZE(sm8250_mdp),
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h
> index 67566b07195a..88288c80b652 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h
> @@ -132,6 +132,8 @@ static const struct dpu_perf_cfg sc7180_perf_data = {
> };
>
> const struct dpu_mdss_cfg dpu_sc7180_cfg = {
> + .core_major_version = 0x6,
> + .core_minor_version = 0x2,
> .caps = &sc7180_dpu_caps,
> .ubwc = &sc7180_ubwc_cfg,
> .mdp_count = ARRAY_SIZE(sc7180_mdp),
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h
> index 031fc8dae3c6..93c901502b5a 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h
> @@ -102,6 +102,8 @@ static const struct dpu_perf_cfg sm6115_perf_data = {
> };
>
> const struct dpu_mdss_cfg dpu_sm6115_cfg = {
> + .core_major_version = 0x6,
> + .core_minor_version = 0x3,
> .caps = &sm6115_dpu_caps,
> .ubwc = &sm6115_ubwc_cfg,
> .mdp_count = ARRAY_SIZE(sm6115_mdp),
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_4_sm6350.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_4_sm6350.h
> index 06eba23b0236..ff7e4b775fd5 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_4_sm6350.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_4_sm6350.h
> @@ -141,6 +141,8 @@ static const struct dpu_perf_cfg sm6350_perf_data = {
> };
>
> const struct dpu_mdss_cfg dpu_sm6350_cfg = {
> + .core_major_version = 0x6,
> + .core_minor_version = 0x4,
> .caps = &sm6350_dpu_caps,
> .ubwc = &sm6350_ubwc_cfg,
> .mdp_count = ARRAY_SIZE(sm6350_mdp),
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h
> index f2808098af39..7bc86aa50e6f 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h
> @@ -92,6 +92,8 @@ static const struct dpu_perf_cfg qcm2290_perf_data = {
> };
>
> const struct dpu_mdss_cfg dpu_qcm2290_cfg = {
> + .core_major_version = 0x6,
> + .core_minor_version = 0x5,
> .caps = &qcm2290_dpu_caps,
> .ubwc = &qcm2290_ubwc_cfg,
> .mdp_count = ARRAY_SIZE(qcm2290_mdp),
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_9_sm6375.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_9_sm6375.h
> index 241fa6746674..d92890f090d4 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_9_sm6375.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_9_sm6375.h
> @@ -107,6 +107,8 @@ static const struct dpu_perf_cfg sm6375_perf_data = {
> };
>
> const struct dpu_mdss_cfg dpu_sm6375_cfg = {
> + .core_major_version = 0x6,
> + .core_minor_version = 0x9,
> .caps = &sm6375_dpu_caps,
> .ubwc = &sm6375_ubwc_cfg,
> .mdp_count = ARRAY_SIZE(sm6375_mdp),
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
> index 8da424eaee6a..8a2dc56c79f8 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
> @@ -213,6 +213,8 @@ static const struct dpu_perf_cfg sm8350_perf_data = {
> };
>
> const struct dpu_mdss_cfg dpu_sm8350_cfg = {
> + .core_major_version = 0x7,
> + .core_minor_version = 0x0,
> .caps = &sm8350_dpu_caps,
> .ubwc = &sm8350_ubwc_cfg,
> .mdp_count = ARRAY_SIZE(sm8350_mdp),
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
> index 900fee410e11..bba7bdb9bd42 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
> @@ -154,6 +154,8 @@ static const struct dpu_perf_cfg sc7280_perf_data = {
> };
>
> const struct dpu_mdss_cfg dpu_sc7280_cfg = {
> + .core_major_version = 0x7,
> + .core_minor_version = 0x2,
> .caps = &sc7280_dpu_caps,
> .ubwc = &sc7280_ubwc_cfg,
> .mdp_count = ARRAY_SIZE(sc7280_mdp),
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
> index f6ce6b090f71..3f51b802b6b7 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
> @@ -217,6 +217,8 @@ static const struct dpu_perf_cfg sc8280xp_perf_data = {
> };
>
> const struct dpu_mdss_cfg dpu_sc8280xp_cfg = {
> + .core_major_version = 0x8,
> + .core_minor_version = 0x0,
> .caps = &sc8280xp_dpu_caps,
> .ubwc = &sc8280xp_ubwc_cfg,
> .mdp_count = ARRAY_SIZE(sc8280xp_mdp),
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
> index 8d13c369213c..20acff9db979 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
> @@ -221,6 +221,8 @@ static const struct dpu_perf_cfg sm8450_perf_data = {
> };
>
> const struct dpu_mdss_cfg dpu_sm8450_cfg = {
> + .core_major_version = 0x8,
> + .core_minor_version = 0x1,
> .caps = &sm8450_dpu_caps,
> .ubwc = &sm8450_ubwc_cfg,
> .mdp_count = ARRAY_SIZE(sm8450_mdp),
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
> index f17b9a7fee85..89fdf334a0aa 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
> @@ -225,6 +225,8 @@ static const struct dpu_perf_cfg sm8550_perf_data = {
> };
>
> const struct dpu_mdss_cfg dpu_sm8550_cfg = {
> + .core_major_version = 0x9,
> + .core_minor_version = 0x0,
> .caps = &sm8550_dpu_caps,
> .ubwc = &sm8550_ubwc_cfg,
> .mdp_count = ARRAY_SIZE(sm8550_mdp),
> 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 b860784ade72..8b900be3ea90 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> @@ -796,8 +796,9 @@ struct dpu_perf_cfg {
> /**
> * struct dpu_mdss_cfg - information of MDSS HW
> * This is the main catalog data structure representing
> - * this HW version. Contains number of instances,
> - * register offsets, capabilities of the all MDSS HW sub-blocks.
> + * this HW version. Contains dpu's major and minor versions,
> + * number of instances, register offsets, capabilities of the
> + * all MDSS HW sub-blocks.
> *
> * @dma_formats Supported formats for dma pipe
> * @cursor_formats Supported formats for cursor pipe
> @@ -805,6 +806,9 @@ struct dpu_perf_cfg {
> * @mdss_irqs: Bitmap with the irqs supported by the target
> */
> struct dpu_mdss_cfg {
> + u8 core_major_version;
> + u8 core_minor_version;
> +
> const struct dpu_caps *caps;
>
> const struct dpu_ubwc_cfg *ubwc;
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 1/3] drm/msm/dpu: re-introduce dpu core revision to the catalog
@ 2023-06-30 0:13 ` Dmitry Baryshkov
0 siblings, 0 replies; 26+ messages in thread
From: Dmitry Baryshkov @ 2023-06-30 0:13 UTC (permalink / raw)
To: Abhinav Kumar, freedreno, Rob Clark, Sean Paul, Marijn Suijten,
David Airlie, Daniel Vetter
Cc: quic_jesszhan, andersson, linux-kernel, dri-devel, linux-arm-msm
On 29/06/2023 22:29, Abhinav Kumar 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;
>
> Since dpu's major and minor versions are now separate fields, lets
> drop all the DPU_HW_VER macros.
>
> [1]: https://patchwork.freedesktop.org/patch/530891/?series=113910&rev=4
>
> changes in v3:
> - drop DPU step version as features are not changing across steps
> - add core_major_version / core_minor_version to avoid conflicts
> - update the commit text to drop references to the dpu macros
>
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h | 2 ++
> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h | 2 ++
> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h | 2 ++
> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h | 2 ++
> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h | 2 ++
> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h | 2 ++
> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h | 2 ++
> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_4_sm6350.h | 2 ++
> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h | 2 ++
> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_9_sm6375.h | 2 ++
> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h | 2 ++
> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h | 2 ++
> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h | 2 ++
> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h | 2 ++
> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h | 2 ++
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 8 ++++++--
> 16 files changed, 36 insertions(+), 2 deletions(-)
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Nit: if there is a need for v-next, I'd probably suggest dropping th hex
prefix from these values.
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
> index 7d0d0e74c3b0..a5d486783c3f 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
> @@ -190,6 +190,8 @@ static const struct dpu_perf_cfg msm8998_perf_data = {
> };
>
> const struct dpu_mdss_cfg dpu_msm8998_cfg = {
> + .core_major_version = 0x3,
> + .core_minor_version = 0x0,
> .caps = &msm8998_dpu_caps,
> .ubwc = &msm8998_ubwc_cfg,
> .mdp_count = ARRAY_SIZE(msm8998_mdp),
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h
> index b6098141bb9b..1fdb89a4b7a6 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h
> @@ -194,6 +194,8 @@ static const struct dpu_perf_cfg sdm845_perf_data = {
> };
>
> const struct dpu_mdss_cfg dpu_sdm845_cfg = {
> + .core_major_version = 0x4,
> + .core_minor_version = 0x0,
> .caps = &sdm845_dpu_caps,
> .ubwc = &sdm845_ubwc_cfg,
> .mdp_count = ARRAY_SIZE(sdm845_mdp),
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h
> index b5f751354267..129c62cf450d 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h
> @@ -208,6 +208,8 @@ static const struct dpu_perf_cfg sm8150_perf_data = {
> };
>
> const struct dpu_mdss_cfg dpu_sm8150_cfg = {
> + .core_major_version = 0x5,
> + .core_minor_version = 0x0,
> .caps = &sm8150_dpu_caps,
> .ubwc = &sm8150_ubwc_cfg,
> .mdp_count = ARRAY_SIZE(sm8150_mdp),
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h
> index 8ed2b263c5ea..ca037b070f44 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h
> @@ -214,6 +214,8 @@ static const struct dpu_perf_cfg sc8180x_perf_data = {
> };
>
> const struct dpu_mdss_cfg dpu_sc8180x_cfg = {
> + .core_major_version = 0x5,
> + .core_minor_version = 0x1,
> .caps = &sc8180x_dpu_caps,
> .ubwc = &sc8180x_ubwc_cfg,
> .mdp_count = ARRAY_SIZE(sc8180x_mdp),
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h
> index daebd2170041..e446af90767e 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h
> @@ -214,6 +214,8 @@ static const struct dpu_perf_cfg sm8250_perf_data = {
> };
>
> const struct dpu_mdss_cfg dpu_sm8250_cfg = {
> + .core_major_version = 0x6,
> + .core_minor_version = 0x0,
> .caps = &sm8250_dpu_caps,
> .ubwc = &sm8250_ubwc_cfg,
> .mdp_count = ARRAY_SIZE(sm8250_mdp),
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h
> index 67566b07195a..88288c80b652 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h
> @@ -132,6 +132,8 @@ static const struct dpu_perf_cfg sc7180_perf_data = {
> };
>
> const struct dpu_mdss_cfg dpu_sc7180_cfg = {
> + .core_major_version = 0x6,
> + .core_minor_version = 0x2,
> .caps = &sc7180_dpu_caps,
> .ubwc = &sc7180_ubwc_cfg,
> .mdp_count = ARRAY_SIZE(sc7180_mdp),
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h
> index 031fc8dae3c6..93c901502b5a 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h
> @@ -102,6 +102,8 @@ static const struct dpu_perf_cfg sm6115_perf_data = {
> };
>
> const struct dpu_mdss_cfg dpu_sm6115_cfg = {
> + .core_major_version = 0x6,
> + .core_minor_version = 0x3,
> .caps = &sm6115_dpu_caps,
> .ubwc = &sm6115_ubwc_cfg,
> .mdp_count = ARRAY_SIZE(sm6115_mdp),
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_4_sm6350.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_4_sm6350.h
> index 06eba23b0236..ff7e4b775fd5 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_4_sm6350.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_4_sm6350.h
> @@ -141,6 +141,8 @@ static const struct dpu_perf_cfg sm6350_perf_data = {
> };
>
> const struct dpu_mdss_cfg dpu_sm6350_cfg = {
> + .core_major_version = 0x6,
> + .core_minor_version = 0x4,
> .caps = &sm6350_dpu_caps,
> .ubwc = &sm6350_ubwc_cfg,
> .mdp_count = ARRAY_SIZE(sm6350_mdp),
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h
> index f2808098af39..7bc86aa50e6f 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h
> @@ -92,6 +92,8 @@ static const struct dpu_perf_cfg qcm2290_perf_data = {
> };
>
> const struct dpu_mdss_cfg dpu_qcm2290_cfg = {
> + .core_major_version = 0x6,
> + .core_minor_version = 0x5,
> .caps = &qcm2290_dpu_caps,
> .ubwc = &qcm2290_ubwc_cfg,
> .mdp_count = ARRAY_SIZE(qcm2290_mdp),
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_9_sm6375.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_9_sm6375.h
> index 241fa6746674..d92890f090d4 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_9_sm6375.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_9_sm6375.h
> @@ -107,6 +107,8 @@ static const struct dpu_perf_cfg sm6375_perf_data = {
> };
>
> const struct dpu_mdss_cfg dpu_sm6375_cfg = {
> + .core_major_version = 0x6,
> + .core_minor_version = 0x9,
> .caps = &sm6375_dpu_caps,
> .ubwc = &sm6375_ubwc_cfg,
> .mdp_count = ARRAY_SIZE(sm6375_mdp),
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
> index 8da424eaee6a..8a2dc56c79f8 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
> @@ -213,6 +213,8 @@ static const struct dpu_perf_cfg sm8350_perf_data = {
> };
>
> const struct dpu_mdss_cfg dpu_sm8350_cfg = {
> + .core_major_version = 0x7,
> + .core_minor_version = 0x0,
> .caps = &sm8350_dpu_caps,
> .ubwc = &sm8350_ubwc_cfg,
> .mdp_count = ARRAY_SIZE(sm8350_mdp),
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
> index 900fee410e11..bba7bdb9bd42 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
> @@ -154,6 +154,8 @@ static const struct dpu_perf_cfg sc7280_perf_data = {
> };
>
> const struct dpu_mdss_cfg dpu_sc7280_cfg = {
> + .core_major_version = 0x7,
> + .core_minor_version = 0x2,
> .caps = &sc7280_dpu_caps,
> .ubwc = &sc7280_ubwc_cfg,
> .mdp_count = ARRAY_SIZE(sc7280_mdp),
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
> index f6ce6b090f71..3f51b802b6b7 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
> @@ -217,6 +217,8 @@ static const struct dpu_perf_cfg sc8280xp_perf_data = {
> };
>
> const struct dpu_mdss_cfg dpu_sc8280xp_cfg = {
> + .core_major_version = 0x8,
> + .core_minor_version = 0x0,
> .caps = &sc8280xp_dpu_caps,
> .ubwc = &sc8280xp_ubwc_cfg,
> .mdp_count = ARRAY_SIZE(sc8280xp_mdp),
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
> index 8d13c369213c..20acff9db979 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
> @@ -221,6 +221,8 @@ static const struct dpu_perf_cfg sm8450_perf_data = {
> };
>
> const struct dpu_mdss_cfg dpu_sm8450_cfg = {
> + .core_major_version = 0x8,
> + .core_minor_version = 0x1,
> .caps = &sm8450_dpu_caps,
> .ubwc = &sm8450_ubwc_cfg,
> .mdp_count = ARRAY_SIZE(sm8450_mdp),
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
> index f17b9a7fee85..89fdf334a0aa 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
> @@ -225,6 +225,8 @@ static const struct dpu_perf_cfg sm8550_perf_data = {
> };
>
> const struct dpu_mdss_cfg dpu_sm8550_cfg = {
> + .core_major_version = 0x9,
> + .core_minor_version = 0x0,
> .caps = &sm8550_dpu_caps,
> .ubwc = &sm8550_ubwc_cfg,
> .mdp_count = ARRAY_SIZE(sm8550_mdp),
> 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 b860784ade72..8b900be3ea90 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> @@ -796,8 +796,9 @@ struct dpu_perf_cfg {
> /**
> * struct dpu_mdss_cfg - information of MDSS HW
> * This is the main catalog data structure representing
> - * this HW version. Contains number of instances,
> - * register offsets, capabilities of the all MDSS HW sub-blocks.
> + * this HW version. Contains dpu's major and minor versions,
> + * number of instances, register offsets, capabilities of the
> + * all MDSS HW sub-blocks.
> *
> * @dma_formats Supported formats for dma pipe
> * @cursor_formats Supported formats for cursor pipe
> @@ -805,6 +806,9 @@ struct dpu_perf_cfg {
> * @mdss_irqs: Bitmap with the irqs supported by the target
> */
> struct dpu_mdss_cfg {
> + u8 core_major_version;
> + u8 core_minor_version;
> +
> const struct dpu_caps *caps;
>
> const struct dpu_ubwc_cfg *ubwc;
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 2/3] drm/msm/dpu: use dpu core's major version to enable data compress
2023-06-29 19:29 ` Abhinav Kumar
@ 2023-06-30 0:20 ` Dmitry Baryshkov
-1 siblings, 0 replies; 26+ messages in thread
From: Dmitry Baryshkov @ 2023-06-30 0:20 UTC (permalink / raw)
To: Abhinav Kumar, freedreno, Rob Clark, Sean Paul, Marijn Suijten,
David Airlie, Daniel Vetter
Cc: dri-devel, quic_jesszhan, andersson, linux-arm-msm, linux-kernel
On 29/06/2023 22:29, Abhinav Kumar wrote:
> Instead of using a feature bit to decide whether to enable data
> compress or not for DSC use-cases, use dpu core's major version instead.
> This will avoid defining feature bits for every bit level details of
> registers.
>
> Also, rename the intf's enable_compression() op to program_datapath()
> and allow it to accept a struct intf_dpu_datapath_cfg to program
> all the bits at once. This can be re-used by widebus later on as
> well as it touches the same register.
Two separate commits please, because...
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
> .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 9 +++++++--
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 9 +++++----
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 16 ++++++++++++++--
> 3 files changed, 26 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> index b856c6286c85..f4e15b4c4cc9 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> @@ -50,6 +50,7 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
> to_dpu_encoder_phys_cmd(phys_enc);
> struct dpu_hw_ctl *ctl;
> struct dpu_hw_intf_cfg intf_cfg = { 0 };
> + struct dpu_kms *dpu_kms = phys_enc->dpu_kms;
>
> ctl = phys_enc->hw_ctl;
> if (!ctl->ops.setup_intf_cfg)
> @@ -68,8 +69,12 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
> phys_enc->hw_intf,
> phys_enc->hw_pp->idx);
>
> - if (intf_cfg.dsc != 0 && phys_enc->hw_intf->ops.enable_compression)
> - phys_enc->hw_intf->ops.enable_compression(phys_enc->hw_intf);
> + if (intf_cfg.dsc != 0 && dpu_kms->catalog->core_major_version >= 0x7) {
... because this becomes incorrect. The datapath should be programmed in
all the cases, if there is a corresponding callback. intf_cfg.dsc should
be used as a condition to set datapath_cfg.
> + struct intf_dpu_datapath_cfg datapath_cfg = { 0 };
No need for `0' in the init, empty braces would be enough.
> +
> + datapath_cfg.data_compress = true;
> + phys_enc->hw_intf->ops.program_datapath(phys_enc->hw_intf, &datapath_cfg);
> + }
> }
>
> static void dpu_encoder_phys_cmd_pp_tx_done_irq(void *arg, int irq_idx)
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> index 5b0f6627e29b..85333df08fbc 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> @@ -513,11 +513,13 @@ static void dpu_hw_intf_disable_autorefresh(struct dpu_hw_intf *intf,
>
> }
>
> -static void dpu_hw_intf_enable_compression(struct dpu_hw_intf *ctx)
> +static void dpu_hw_intf_program_datapath(struct dpu_hw_intf *ctx,
> + struct intf_dpu_datapath_cfg *datapath_cfg)
> {
> u32 intf_cfg2 = DPU_REG_READ(&ctx->hw, INTF_CONFIG2);
>
> - intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
> + if (datapath_cfg->data_compress)
> + intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
>
> DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, intf_cfg2);
> }
> @@ -543,8 +545,7 @@ static void _setup_intf_ops(struct dpu_hw_intf_ops *ops,
> ops->disable_autorefresh = dpu_hw_intf_disable_autorefresh;
> }
>
> - if (cap & BIT(DPU_INTF_DATA_COMPRESS))
> - ops->enable_compression = dpu_hw_intf_enable_compression;
> + ops->program_datapath = dpu_hw_intf_program_datapath;
The `core_major_version >= 7' should either be here or in the callback
itself.
> }
>
> struct dpu_hw_intf *dpu_hw_intf_init(const struct dpu_intf_cfg *cfg,
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> index 99e21c4137f9..f736dca38463 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> @@ -48,6 +48,11 @@ struct intf_status {
> u32 line_count; /* current line count including blanking */
> };
>
> +struct intf_dpu_datapath_cfg {
> + u8 data_compress; /* enable data compress between dpu and dsi */
> + /* can be expanded for other programmable bits */
> +};
I'd say, dpu_datapath is too generic. What about intf_cmd_mode_cfg?
> +
> /**
> * struct dpu_hw_intf_ops : Interface to the interface Hw driver functions
> * Assumption is these functions will be called after clocks are enabled
> @@ -70,7 +75,7 @@ struct intf_status {
> * @get_autorefresh: Retrieve autorefresh config from hardware
> * Return: 0 on success, -ETIMEDOUT on timeout
> * @vsync_sel: Select vsync signal for tear-effect configuration
> - * @enable_compression: Enable data compression
> + * @program_datapath: Program the DPU to interface datapath for relevant chipsets
> */
> struct dpu_hw_intf_ops {
> void (*setup_timing_gen)(struct dpu_hw_intf *intf,
> @@ -108,7 +113,14 @@ struct dpu_hw_intf_ops {
> */
> void (*disable_autorefresh)(struct dpu_hw_intf *intf, uint32_t encoder_id, u16 vdisplay);
>
> - void (*enable_compression)(struct dpu_hw_intf *intf);
> + /**
> + * Program the DPU to intf datapath by specifying
> + * which of the settings need to be programmed for
> + * use-cases which need DPU-intf handshake such as
> + * widebus, compression etc.
This is not a valid kerneldoc.
> + */
> + void (*program_datapath)(struct dpu_hw_intf *intf,
> + struct intf_dpu_datapath_cfg *datapath_cfg);
> };
>
> struct dpu_hw_intf {
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 2/3] drm/msm/dpu: use dpu core's major version to enable data compress
@ 2023-06-30 0:20 ` Dmitry Baryshkov
0 siblings, 0 replies; 26+ messages in thread
From: Dmitry Baryshkov @ 2023-06-30 0:20 UTC (permalink / raw)
To: Abhinav Kumar, freedreno, Rob Clark, Sean Paul, Marijn Suijten,
David Airlie, Daniel Vetter
Cc: quic_jesszhan, andersson, linux-kernel, dri-devel, linux-arm-msm
On 29/06/2023 22:29, Abhinav Kumar wrote:
> Instead of using a feature bit to decide whether to enable data
> compress or not for DSC use-cases, use dpu core's major version instead.
> This will avoid defining feature bits for every bit level details of
> registers.
>
> Also, rename the intf's enable_compression() op to program_datapath()
> and allow it to accept a struct intf_dpu_datapath_cfg to program
> all the bits at once. This can be re-used by widebus later on as
> well as it touches the same register.
Two separate commits please, because...
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
> .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 9 +++++++--
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 9 +++++----
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 16 ++++++++++++++--
> 3 files changed, 26 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> index b856c6286c85..f4e15b4c4cc9 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> @@ -50,6 +50,7 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
> to_dpu_encoder_phys_cmd(phys_enc);
> struct dpu_hw_ctl *ctl;
> struct dpu_hw_intf_cfg intf_cfg = { 0 };
> + struct dpu_kms *dpu_kms = phys_enc->dpu_kms;
>
> ctl = phys_enc->hw_ctl;
> if (!ctl->ops.setup_intf_cfg)
> @@ -68,8 +69,12 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
> phys_enc->hw_intf,
> phys_enc->hw_pp->idx);
>
> - if (intf_cfg.dsc != 0 && phys_enc->hw_intf->ops.enable_compression)
> - phys_enc->hw_intf->ops.enable_compression(phys_enc->hw_intf);
> + if (intf_cfg.dsc != 0 && dpu_kms->catalog->core_major_version >= 0x7) {
... because this becomes incorrect. The datapath should be programmed in
all the cases, if there is a corresponding callback. intf_cfg.dsc should
be used as a condition to set datapath_cfg.
> + struct intf_dpu_datapath_cfg datapath_cfg = { 0 };
No need for `0' in the init, empty braces would be enough.
> +
> + datapath_cfg.data_compress = true;
> + phys_enc->hw_intf->ops.program_datapath(phys_enc->hw_intf, &datapath_cfg);
> + }
> }
>
> static void dpu_encoder_phys_cmd_pp_tx_done_irq(void *arg, int irq_idx)
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> index 5b0f6627e29b..85333df08fbc 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> @@ -513,11 +513,13 @@ static void dpu_hw_intf_disable_autorefresh(struct dpu_hw_intf *intf,
>
> }
>
> -static void dpu_hw_intf_enable_compression(struct dpu_hw_intf *ctx)
> +static void dpu_hw_intf_program_datapath(struct dpu_hw_intf *ctx,
> + struct intf_dpu_datapath_cfg *datapath_cfg)
> {
> u32 intf_cfg2 = DPU_REG_READ(&ctx->hw, INTF_CONFIG2);
>
> - intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
> + if (datapath_cfg->data_compress)
> + intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
>
> DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, intf_cfg2);
> }
> @@ -543,8 +545,7 @@ static void _setup_intf_ops(struct dpu_hw_intf_ops *ops,
> ops->disable_autorefresh = dpu_hw_intf_disable_autorefresh;
> }
>
> - if (cap & BIT(DPU_INTF_DATA_COMPRESS))
> - ops->enable_compression = dpu_hw_intf_enable_compression;
> + ops->program_datapath = dpu_hw_intf_program_datapath;
The `core_major_version >= 7' should either be here or in the callback
itself.
> }
>
> struct dpu_hw_intf *dpu_hw_intf_init(const struct dpu_intf_cfg *cfg,
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> index 99e21c4137f9..f736dca38463 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> @@ -48,6 +48,11 @@ struct intf_status {
> u32 line_count; /* current line count including blanking */
> };
>
> +struct intf_dpu_datapath_cfg {
> + u8 data_compress; /* enable data compress between dpu and dsi */
> + /* can be expanded for other programmable bits */
> +};
I'd say, dpu_datapath is too generic. What about intf_cmd_mode_cfg?
> +
> /**
> * struct dpu_hw_intf_ops : Interface to the interface Hw driver functions
> * Assumption is these functions will be called after clocks are enabled
> @@ -70,7 +75,7 @@ struct intf_status {
> * @get_autorefresh: Retrieve autorefresh config from hardware
> * Return: 0 on success, -ETIMEDOUT on timeout
> * @vsync_sel: Select vsync signal for tear-effect configuration
> - * @enable_compression: Enable data compression
> + * @program_datapath: Program the DPU to interface datapath for relevant chipsets
> */
> struct dpu_hw_intf_ops {
> void (*setup_timing_gen)(struct dpu_hw_intf *intf,
> @@ -108,7 +113,14 @@ struct dpu_hw_intf_ops {
> */
> void (*disable_autorefresh)(struct dpu_hw_intf *intf, uint32_t encoder_id, u16 vdisplay);
>
> - void (*enable_compression)(struct dpu_hw_intf *intf);
> + /**
> + * Program the DPU to intf datapath by specifying
> + * which of the settings need to be programmed for
> + * use-cases which need DPU-intf handshake such as
> + * widebus, compression etc.
This is not a valid kerneldoc.
> + */
> + void (*program_datapath)(struct dpu_hw_intf *intf,
> + struct intf_dpu_datapath_cfg *datapath_cfg);
> };
>
> struct dpu_hw_intf {
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 3/3] drm/msm/dpu: drop DPU_INTF_DATA_COMPRESS from dpu catalog
2023-06-29 19:29 ` Abhinav Kumar
@ 2023-06-30 0:20 ` Dmitry Baryshkov
-1 siblings, 0 replies; 26+ messages in thread
From: Dmitry Baryshkov @ 2023-06-30 0:20 UTC (permalink / raw)
To: Abhinav Kumar, freedreno, Rob Clark, Sean Paul, Marijn Suijten,
David Airlie, Daniel Vetter
Cc: dri-devel, quic_jesszhan, andersson, linux-arm-msm, linux-kernel
On 29/06/2023 22:29, Abhinav Kumar wrote:
> Now that all usages of DPU_INTF_DATA_COMPRESS have been replaced
> with the dpu core's major revision lets drop DPU_INTF_DATA_COMPRESS
> from the catalog completely.
>
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 2 +-
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 --
> 2 files changed, 1 insertion(+), 3 deletions(-)
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 3/3] drm/msm/dpu: drop DPU_INTF_DATA_COMPRESS from dpu catalog
@ 2023-06-30 0:20 ` Dmitry Baryshkov
0 siblings, 0 replies; 26+ messages in thread
From: Dmitry Baryshkov @ 2023-06-30 0:20 UTC (permalink / raw)
To: Abhinav Kumar, freedreno, Rob Clark, Sean Paul, Marijn Suijten,
David Airlie, Daniel Vetter
Cc: quic_jesszhan, andersson, linux-kernel, dri-devel, linux-arm-msm
On 29/06/2023 22:29, Abhinav Kumar wrote:
> Now that all usages of DPU_INTF_DATA_COMPRESS have been replaced
> with the dpu core's major revision lets drop DPU_INTF_DATA_COMPRESS
> from the catalog completely.
>
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 2 +-
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 --
> 2 files changed, 1 insertion(+), 3 deletions(-)
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 1/3] drm/msm/dpu: re-introduce dpu core revision to the catalog
2023-06-29 19:29 ` Abhinav Kumar
@ 2023-06-30 0:24 ` Dmitry Baryshkov
-1 siblings, 0 replies; 26+ messages in thread
From: Dmitry Baryshkov @ 2023-06-30 0:24 UTC (permalink / raw)
To: Abhinav Kumar, freedreno, Rob Clark, Sean Paul, Marijn Suijten,
David Airlie, Daniel Vetter
Cc: dri-devel, quic_jesszhan, andersson, linux-arm-msm, linux-kernel
On 29/06/2023 22:29, Abhinav Kumar 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.
Generic note: this description can be moved to the cover letter, it
covers the series intent.
> 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;
>
> Since dpu's major and minor versions are now separate fields, lets
> drop all the DPU_HW_VER macros.
>
> [1]: https://patchwork.freedesktop.org/patch/530891/?series=113910&rev=4
Please use `commit aabbcc ("do this and that")' in the commit messages.
>
> changes in v3:
> - drop DPU step version as features are not changing across steps
> - add core_major_version / core_minor_version to avoid conflicts
> - update the commit text to drop references to the dpu macros
>
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 1/3] drm/msm/dpu: re-introduce dpu core revision to the catalog
@ 2023-06-30 0:24 ` Dmitry Baryshkov
0 siblings, 0 replies; 26+ messages in thread
From: Dmitry Baryshkov @ 2023-06-30 0:24 UTC (permalink / raw)
To: Abhinav Kumar, freedreno, Rob Clark, Sean Paul, Marijn Suijten,
David Airlie, Daniel Vetter
Cc: quic_jesszhan, andersson, linux-kernel, dri-devel, linux-arm-msm
On 29/06/2023 22:29, Abhinav Kumar 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.
Generic note: this description can be moved to the cover letter, it
covers the series intent.
> 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;
>
> Since dpu's major and minor versions are now separate fields, lets
> drop all the DPU_HW_VER macros.
>
> [1]: https://patchwork.freedesktop.org/patch/530891/?series=113910&rev=4
Please use `commit aabbcc ("do this and that")' in the commit messages.
>
> changes in v3:
> - drop DPU step version as features are not changing across steps
> - add core_major_version / core_minor_version to avoid conflicts
> - update the commit text to drop references to the dpu macros
>
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 2/3] drm/msm/dpu: use dpu core's major version to enable data compress
2023-06-30 0:20 ` Dmitry Baryshkov
@ 2023-06-30 3:07 ` Abhinav Kumar
-1 siblings, 0 replies; 26+ messages in thread
From: Abhinav Kumar @ 2023-06-30 3:07 UTC (permalink / raw)
To: Dmitry Baryshkov, freedreno, Rob Clark, Sean Paul,
Marijn Suijten, David Airlie, Daniel Vetter
Cc: dri-devel, quic_jesszhan, andersson, linux-arm-msm, linux-kernel
On 6/29/2023 5:20 PM, Dmitry Baryshkov wrote:
> On 29/06/2023 22:29, Abhinav Kumar wrote:
>> Instead of using a feature bit to decide whether to enable data
>> compress or not for DSC use-cases, use dpu core's major version instead.
>> This will avoid defining feature bits for every bit level details of
>> registers.
>>
>> Also, rename the intf's enable_compression() op to program_datapath()
>> and allow it to accept a struct intf_dpu_datapath_cfg to program
>> all the bits at once. This can be re-used by widebus later on as
>> well as it touches the same register.
>
> Two separate commits please, because...
>
I thought of it but it seemed better together and was the only way I
could think of. Please see below.
>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>> ---
>> .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 9 +++++++--
>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 9 +++++----
>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 16 ++++++++++++++--
>> 3 files changed, 26 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>> index b856c6286c85..f4e15b4c4cc9 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>> @@ -50,6 +50,7 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
>> to_dpu_encoder_phys_cmd(phys_enc);
>> struct dpu_hw_ctl *ctl;
>> struct dpu_hw_intf_cfg intf_cfg = { 0 };
>> + struct dpu_kms *dpu_kms = phys_enc->dpu_kms;
>> ctl = phys_enc->hw_ctl;
>> if (!ctl->ops.setup_intf_cfg)
>> @@ -68,8 +69,12 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
>> phys_enc->hw_intf,
>> phys_enc->hw_pp->idx);
>> - if (intf_cfg.dsc != 0 && phys_enc->hw_intf->ops.enable_compression)
>> - phys_enc->hw_intf->ops.enable_compression(phys_enc->hw_intf);
>> + if (intf_cfg.dsc != 0 && dpu_kms->catalog->core_major_version >=
>> 0x7) {
>
> ... because this becomes incorrect. The datapath should be programmed in
> all the cases, if there is a corresponding callback. intf_cfg.dsc should
> be used as a condition to set datapath_cfg.
>
The issue is that today we do not have dpu_mdss_cfg as part of
dpu_hw_intf nor _setup_intf_ops because all of those have been dropped
with some rework or cleanup.
Ideally even I would like to assign this op only for core_rev >=7 but
that information is no longer available. We would have to start passing
the major and minor versions to _setup_intf_ops() to go with that
approach. So without making all of those changes, the only way I had was
to assign the op unconditionally but call it only for major_rev >= 7.
Passing core_rev to the op itself so that we can write the register only
for core_rev >=7 is an option but then what if some bits start becoming
usable only after minor rev. then we will have to start passing major
and minor rev to program_datapath too. Again getting little messy.
I am open to ideas to achieve the goal of assigning this op only for
core_rev >=7 other than what I wrote above.
>
>> + struct intf_dpu_datapath_cfg datapath_cfg = { 0 };
>
> No need for `0' in the init, empty braces would be enough.
>
ack.
>> +
>> + datapath_cfg.data_compress = true;
>> + phys_enc->hw_intf->ops.program_datapath(phys_enc->hw_intf,
>> &datapath_cfg);
>> + }
>> }
>> static void dpu_encoder_phys_cmd_pp_tx_done_irq(void *arg, int irq_idx)
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>> index 5b0f6627e29b..85333df08fbc 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>> @@ -513,11 +513,13 @@ static void
>> dpu_hw_intf_disable_autorefresh(struct dpu_hw_intf *intf,
>> }
>> -static void dpu_hw_intf_enable_compression(struct dpu_hw_intf *ctx)
>> +static void dpu_hw_intf_program_datapath(struct dpu_hw_intf *ctx,
>> + struct intf_dpu_datapath_cfg *datapath_cfg)
>> {
>> u32 intf_cfg2 = DPU_REG_READ(&ctx->hw, INTF_CONFIG2);
>> - intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
>> + if (datapath_cfg->data_compress)
>> + intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
>> DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, intf_cfg2);
>> }
>> @@ -543,8 +545,7 @@ static void _setup_intf_ops(struct dpu_hw_intf_ops
>> *ops,
>> ops->disable_autorefresh = dpu_hw_intf_disable_autorefresh;
>> }
>> - if (cap & BIT(DPU_INTF_DATA_COMPRESS))
>> - ops->enable_compression = dpu_hw_intf_enable_compression;
>> + ops->program_datapath = dpu_hw_intf_program_datapath;
>
> The `core_major_version >= 7' should either be here or in the callback
> itself.
>
Yes, ideally I would like it like that but please see above why I
couldnt do it.
>> }
>> struct dpu_hw_intf *dpu_hw_intf_init(const struct dpu_intf_cfg *cfg,
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>> index 99e21c4137f9..f736dca38463 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>> @@ -48,6 +48,11 @@ struct intf_status {
>> u32 line_count; /* current line count including blanking */
>> };
>> +struct intf_dpu_datapath_cfg {
>> + u8 data_compress; /* enable data compress between dpu and dsi */
>> + /* can be expanded for other programmable bits */
>> +};
>
> I'd say, dpu_datapath is too generic. What about intf_cmd_mode_cfg?
>
The goal was to keep it generic. Its actually the handshake between DPU
and interface datapath so I chose that name.
This is not specific to command mode and intf_cfg is already there so I
chose that one :)
>> +
>> /**
>> * struct dpu_hw_intf_ops : Interface to the interface Hw driver
>> functions
>> * Assumption is these functions will be called after clocks are
>> enabled
>> @@ -70,7 +75,7 @@ struct intf_status {
>> * @get_autorefresh: Retrieve autorefresh config from
>> hardware
>> * Return: 0 on success, -ETIMEDOUT on
>> timeout
>> * @vsync_sel: Select vsync signal for tear-effect
>> configuration
>> - * @enable_compression: Enable data compression
>> + * @program_datapath: Program the DPU to interface datapath
>> for relevant chipsets
>> */
>> struct dpu_hw_intf_ops {
>> void (*setup_timing_gen)(struct dpu_hw_intf *intf,
>> @@ -108,7 +113,14 @@ struct dpu_hw_intf_ops {
>> */
>> void (*disable_autorefresh)(struct dpu_hw_intf *intf, uint32_t
>> encoder_id, u16 vdisplay);
>> - void (*enable_compression)(struct dpu_hw_intf *intf);
>> + /**
>> + * Program the DPU to intf datapath by specifying
>> + * which of the settings need to be programmed for
>> + * use-cases which need DPU-intf handshake such as
>> + * widebus, compression etc.
>
> This is not a valid kerneldoc.
>
hmmm ... ok so just // ?
I referred disable_autorefresh from above and did the same.
>> + */
>> + void (*program_datapath)(struct dpu_hw_intf *intf,
>> + struct intf_dpu_datapath_cfg *datapath_cfg);
>> };
>> struct dpu_hw_intf {
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 2/3] drm/msm/dpu: use dpu core's major version to enable data compress
@ 2023-06-30 3:07 ` Abhinav Kumar
0 siblings, 0 replies; 26+ messages in thread
From: Abhinav Kumar @ 2023-06-30 3:07 UTC (permalink / raw)
To: Dmitry Baryshkov, freedreno, Rob Clark, Sean Paul,
Marijn Suijten, David Airlie, Daniel Vetter
Cc: quic_jesszhan, andersson, linux-kernel, dri-devel, linux-arm-msm
On 6/29/2023 5:20 PM, Dmitry Baryshkov wrote:
> On 29/06/2023 22:29, Abhinav Kumar wrote:
>> Instead of using a feature bit to decide whether to enable data
>> compress or not for DSC use-cases, use dpu core's major version instead.
>> This will avoid defining feature bits for every bit level details of
>> registers.
>>
>> Also, rename the intf's enable_compression() op to program_datapath()
>> and allow it to accept a struct intf_dpu_datapath_cfg to program
>> all the bits at once. This can be re-used by widebus later on as
>> well as it touches the same register.
>
> Two separate commits please, because...
>
I thought of it but it seemed better together and was the only way I
could think of. Please see below.
>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>> ---
>> .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 9 +++++++--
>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 9 +++++----
>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 16 ++++++++++++++--
>> 3 files changed, 26 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>> index b856c6286c85..f4e15b4c4cc9 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>> @@ -50,6 +50,7 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
>> to_dpu_encoder_phys_cmd(phys_enc);
>> struct dpu_hw_ctl *ctl;
>> struct dpu_hw_intf_cfg intf_cfg = { 0 };
>> + struct dpu_kms *dpu_kms = phys_enc->dpu_kms;
>> ctl = phys_enc->hw_ctl;
>> if (!ctl->ops.setup_intf_cfg)
>> @@ -68,8 +69,12 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
>> phys_enc->hw_intf,
>> phys_enc->hw_pp->idx);
>> - if (intf_cfg.dsc != 0 && phys_enc->hw_intf->ops.enable_compression)
>> - phys_enc->hw_intf->ops.enable_compression(phys_enc->hw_intf);
>> + if (intf_cfg.dsc != 0 && dpu_kms->catalog->core_major_version >=
>> 0x7) {
>
> ... because this becomes incorrect. The datapath should be programmed in
> all the cases, if there is a corresponding callback. intf_cfg.dsc should
> be used as a condition to set datapath_cfg.
>
The issue is that today we do not have dpu_mdss_cfg as part of
dpu_hw_intf nor _setup_intf_ops because all of those have been dropped
with some rework or cleanup.
Ideally even I would like to assign this op only for core_rev >=7 but
that information is no longer available. We would have to start passing
the major and minor versions to _setup_intf_ops() to go with that
approach. So without making all of those changes, the only way I had was
to assign the op unconditionally but call it only for major_rev >= 7.
Passing core_rev to the op itself so that we can write the register only
for core_rev >=7 is an option but then what if some bits start becoming
usable only after minor rev. then we will have to start passing major
and minor rev to program_datapath too. Again getting little messy.
I am open to ideas to achieve the goal of assigning this op only for
core_rev >=7 other than what I wrote above.
>
>> + struct intf_dpu_datapath_cfg datapath_cfg = { 0 };
>
> No need for `0' in the init, empty braces would be enough.
>
ack.
>> +
>> + datapath_cfg.data_compress = true;
>> + phys_enc->hw_intf->ops.program_datapath(phys_enc->hw_intf,
>> &datapath_cfg);
>> + }
>> }
>> static void dpu_encoder_phys_cmd_pp_tx_done_irq(void *arg, int irq_idx)
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>> index 5b0f6627e29b..85333df08fbc 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>> @@ -513,11 +513,13 @@ static void
>> dpu_hw_intf_disable_autorefresh(struct dpu_hw_intf *intf,
>> }
>> -static void dpu_hw_intf_enable_compression(struct dpu_hw_intf *ctx)
>> +static void dpu_hw_intf_program_datapath(struct dpu_hw_intf *ctx,
>> + struct intf_dpu_datapath_cfg *datapath_cfg)
>> {
>> u32 intf_cfg2 = DPU_REG_READ(&ctx->hw, INTF_CONFIG2);
>> - intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
>> + if (datapath_cfg->data_compress)
>> + intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
>> DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, intf_cfg2);
>> }
>> @@ -543,8 +545,7 @@ static void _setup_intf_ops(struct dpu_hw_intf_ops
>> *ops,
>> ops->disable_autorefresh = dpu_hw_intf_disable_autorefresh;
>> }
>> - if (cap & BIT(DPU_INTF_DATA_COMPRESS))
>> - ops->enable_compression = dpu_hw_intf_enable_compression;
>> + ops->program_datapath = dpu_hw_intf_program_datapath;
>
> The `core_major_version >= 7' should either be here or in the callback
> itself.
>
Yes, ideally I would like it like that but please see above why I
couldnt do it.
>> }
>> struct dpu_hw_intf *dpu_hw_intf_init(const struct dpu_intf_cfg *cfg,
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>> index 99e21c4137f9..f736dca38463 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>> @@ -48,6 +48,11 @@ struct intf_status {
>> u32 line_count; /* current line count including blanking */
>> };
>> +struct intf_dpu_datapath_cfg {
>> + u8 data_compress; /* enable data compress between dpu and dsi */
>> + /* can be expanded for other programmable bits */
>> +};
>
> I'd say, dpu_datapath is too generic. What about intf_cmd_mode_cfg?
>
The goal was to keep it generic. Its actually the handshake between DPU
and interface datapath so I chose that name.
This is not specific to command mode and intf_cfg is already there so I
chose that one :)
>> +
>> /**
>> * struct dpu_hw_intf_ops : Interface to the interface Hw driver
>> functions
>> * Assumption is these functions will be called after clocks are
>> enabled
>> @@ -70,7 +75,7 @@ struct intf_status {
>> * @get_autorefresh: Retrieve autorefresh config from
>> hardware
>> * Return: 0 on success, -ETIMEDOUT on
>> timeout
>> * @vsync_sel: Select vsync signal for tear-effect
>> configuration
>> - * @enable_compression: Enable data compression
>> + * @program_datapath: Program the DPU to interface datapath
>> for relevant chipsets
>> */
>> struct dpu_hw_intf_ops {
>> void (*setup_timing_gen)(struct dpu_hw_intf *intf,
>> @@ -108,7 +113,14 @@ struct dpu_hw_intf_ops {
>> */
>> void (*disable_autorefresh)(struct dpu_hw_intf *intf, uint32_t
>> encoder_id, u16 vdisplay);
>> - void (*enable_compression)(struct dpu_hw_intf *intf);
>> + /**
>> + * Program the DPU to intf datapath by specifying
>> + * which of the settings need to be programmed for
>> + * use-cases which need DPU-intf handshake such as
>> + * widebus, compression etc.
>
> This is not a valid kerneldoc.
>
hmmm ... ok so just // ?
I referred disable_autorefresh from above and did the same.
>> + */
>> + void (*program_datapath)(struct dpu_hw_intf *intf,
>> + struct intf_dpu_datapath_cfg *datapath_cfg);
>> };
>> struct dpu_hw_intf {
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 1/3] drm/msm/dpu: re-introduce dpu core revision to the catalog
2023-06-30 0:13 ` Dmitry Baryshkov
@ 2023-06-30 3:15 ` Abhinav Kumar
-1 siblings, 0 replies; 26+ messages in thread
From: Abhinav Kumar @ 2023-06-30 3:15 UTC (permalink / raw)
To: Dmitry Baryshkov, freedreno, Rob Clark, Sean Paul,
Marijn Suijten, David Airlie, Daniel Vetter
Cc: dri-devel, quic_jesszhan, andersson, linux-arm-msm, linux-kernel
On 6/29/2023 5:13 PM, Dmitry Baryshkov wrote:
> On 29/06/2023 22:29, Abhinav Kumar 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;
>>
>> Since dpu's major and minor versions are now separate fields, lets
>> drop all the DPU_HW_VER macros.
>>
>> [1]: https://patchwork.freedesktop.org/patch/530891/?series=113910&rev=4
>>
>> changes in v3:
>> - drop DPU step version as features are not changing across steps
>> - add core_major_version / core_minor_version to avoid conflicts
>> - update the commit text to drop references to the dpu macros
>>
>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>> ---
>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h | 2 ++
>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h | 2 ++
>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h | 2 ++
>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h | 2 ++
>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h | 2 ++
>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h | 2 ++
>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h | 2 ++
>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_4_sm6350.h | 2 ++
>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h | 2 ++
>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_9_sm6375.h | 2 ++
>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h | 2 ++
>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h | 2 ++
>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h | 2 ++
>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h | 2 ++
>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h | 2 ++
>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 8 ++++++--
>> 16 files changed, 36 insertions(+), 2 deletions(-)
>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>
> Nit: if there is a need for v-next, I'd probably suggest dropping th hex
> prefix from these values.
>
ack, I was debating in my head whether to keep it or not and kept it in
the end. I am fine to drop it too.
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
>> b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
>> index 7d0d0e74c3b0..a5d486783c3f 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
>> @@ -190,6 +190,8 @@ static const struct dpu_perf_cfg msm8998_perf_data
>> = {
>> };
>> const struct dpu_mdss_cfg dpu_msm8998_cfg = {
>> + .core_major_version = 0x3,
>> + .core_minor_version = 0x0,
>> .caps = &msm8998_dpu_caps,
>> .ubwc = &msm8998_ubwc_cfg,
>> .mdp_count = ARRAY_SIZE(msm8998_mdp),
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h
>> b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h
>> index b6098141bb9b..1fdb89a4b7a6 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h
>> @@ -194,6 +194,8 @@ static const struct dpu_perf_cfg sdm845_perf_data = {
>> };
>> const struct dpu_mdss_cfg dpu_sdm845_cfg = {
>> + .core_major_version = 0x4,
>> + .core_minor_version = 0x0,
>> .caps = &sdm845_dpu_caps,
>> .ubwc = &sdm845_ubwc_cfg,
>> .mdp_count = ARRAY_SIZE(sdm845_mdp),
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h
>> b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h
>> index b5f751354267..129c62cf450d 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h
>> @@ -208,6 +208,8 @@ static const struct dpu_perf_cfg sm8150_perf_data = {
>> };
>> const struct dpu_mdss_cfg dpu_sm8150_cfg = {
>> + .core_major_version = 0x5,
>> + .core_minor_version = 0x0,
>> .caps = &sm8150_dpu_caps,
>> .ubwc = &sm8150_ubwc_cfg,
>> .mdp_count = ARRAY_SIZE(sm8150_mdp),
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h
>> b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h
>> index 8ed2b263c5ea..ca037b070f44 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h
>> @@ -214,6 +214,8 @@ static const struct dpu_perf_cfg sc8180x_perf_data
>> = {
>> };
>> const struct dpu_mdss_cfg dpu_sc8180x_cfg = {
>> + .core_major_version = 0x5,
>> + .core_minor_version = 0x1,
>> .caps = &sc8180x_dpu_caps,
>> .ubwc = &sc8180x_ubwc_cfg,
>> .mdp_count = ARRAY_SIZE(sc8180x_mdp),
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h
>> b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h
>> index daebd2170041..e446af90767e 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h
>> @@ -214,6 +214,8 @@ static const struct dpu_perf_cfg sm8250_perf_data = {
>> };
>> const struct dpu_mdss_cfg dpu_sm8250_cfg = {
>> + .core_major_version = 0x6,
>> + .core_minor_version = 0x0,
>> .caps = &sm8250_dpu_caps,
>> .ubwc = &sm8250_ubwc_cfg,
>> .mdp_count = ARRAY_SIZE(sm8250_mdp),
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h
>> b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h
>> index 67566b07195a..88288c80b652 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h
>> @@ -132,6 +132,8 @@ static const struct dpu_perf_cfg sc7180_perf_data = {
>> };
>> const struct dpu_mdss_cfg dpu_sc7180_cfg = {
>> + .core_major_version = 0x6,
>> + .core_minor_version = 0x2,
>> .caps = &sc7180_dpu_caps,
>> .ubwc = &sc7180_ubwc_cfg,
>> .mdp_count = ARRAY_SIZE(sc7180_mdp),
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h
>> b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h
>> index 031fc8dae3c6..93c901502b5a 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h
>> @@ -102,6 +102,8 @@ static const struct dpu_perf_cfg sm6115_perf_data = {
>> };
>> const struct dpu_mdss_cfg dpu_sm6115_cfg = {
>> + .core_major_version = 0x6,
>> + .core_minor_version = 0x3,
>> .caps = &sm6115_dpu_caps,
>> .ubwc = &sm6115_ubwc_cfg,
>> .mdp_count = ARRAY_SIZE(sm6115_mdp),
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_4_sm6350.h
>> b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_4_sm6350.h
>> index 06eba23b0236..ff7e4b775fd5 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_4_sm6350.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_4_sm6350.h
>> @@ -141,6 +141,8 @@ static const struct dpu_perf_cfg sm6350_perf_data = {
>> };
>> const struct dpu_mdss_cfg dpu_sm6350_cfg = {
>> + .core_major_version = 0x6,
>> + .core_minor_version = 0x4,
>> .caps = &sm6350_dpu_caps,
>> .ubwc = &sm6350_ubwc_cfg,
>> .mdp_count = ARRAY_SIZE(sm6350_mdp),
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h
>> b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h
>> index f2808098af39..7bc86aa50e6f 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h
>> @@ -92,6 +92,8 @@ static const struct dpu_perf_cfg qcm2290_perf_data = {
>> };
>> const struct dpu_mdss_cfg dpu_qcm2290_cfg = {
>> + .core_major_version = 0x6,
>> + .core_minor_version = 0x5,
>> .caps = &qcm2290_dpu_caps,
>> .ubwc = &qcm2290_ubwc_cfg,
>> .mdp_count = ARRAY_SIZE(qcm2290_mdp),
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_9_sm6375.h
>> b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_9_sm6375.h
>> index 241fa6746674..d92890f090d4 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_9_sm6375.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_9_sm6375.h
>> @@ -107,6 +107,8 @@ static const struct dpu_perf_cfg sm6375_perf_data = {
>> };
>> const struct dpu_mdss_cfg dpu_sm6375_cfg = {
>> + .core_major_version = 0x6,
>> + .core_minor_version = 0x9,
>> .caps = &sm6375_dpu_caps,
>> .ubwc = &sm6375_ubwc_cfg,
>> .mdp_count = ARRAY_SIZE(sm6375_mdp),
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
>> b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
>> index 8da424eaee6a..8a2dc56c79f8 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
>> @@ -213,6 +213,8 @@ static const struct dpu_perf_cfg sm8350_perf_data = {
>> };
>> const struct dpu_mdss_cfg dpu_sm8350_cfg = {
>> + .core_major_version = 0x7,
>> + .core_minor_version = 0x0,
>> .caps = &sm8350_dpu_caps,
>> .ubwc = &sm8350_ubwc_cfg,
>> .mdp_count = ARRAY_SIZE(sm8350_mdp),
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
>> b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
>> index 900fee410e11..bba7bdb9bd42 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
>> @@ -154,6 +154,8 @@ static const struct dpu_perf_cfg sc7280_perf_data = {
>> };
>> const struct dpu_mdss_cfg dpu_sc7280_cfg = {
>> + .core_major_version = 0x7,
>> + .core_minor_version = 0x2,
>> .caps = &sc7280_dpu_caps,
>> .ubwc = &sc7280_ubwc_cfg,
>> .mdp_count = ARRAY_SIZE(sc7280_mdp),
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
>> b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
>> index f6ce6b090f71..3f51b802b6b7 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
>> @@ -217,6 +217,8 @@ static const struct dpu_perf_cfg
>> sc8280xp_perf_data = {
>> };
>> const struct dpu_mdss_cfg dpu_sc8280xp_cfg = {
>> + .core_major_version = 0x8,
>> + .core_minor_version = 0x0,
>> .caps = &sc8280xp_dpu_caps,
>> .ubwc = &sc8280xp_ubwc_cfg,
>> .mdp_count = ARRAY_SIZE(sc8280xp_mdp),
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
>> b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
>> index 8d13c369213c..20acff9db979 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
>> @@ -221,6 +221,8 @@ static const struct dpu_perf_cfg sm8450_perf_data = {
>> };
>> const struct dpu_mdss_cfg dpu_sm8450_cfg = {
>> + .core_major_version = 0x8,
>> + .core_minor_version = 0x1,
>> .caps = &sm8450_dpu_caps,
>> .ubwc = &sm8450_ubwc_cfg,
>> .mdp_count = ARRAY_SIZE(sm8450_mdp),
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
>> b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
>> index f17b9a7fee85..89fdf334a0aa 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
>> @@ -225,6 +225,8 @@ static const struct dpu_perf_cfg sm8550_perf_data = {
>> };
>> const struct dpu_mdss_cfg dpu_sm8550_cfg = {
>> + .core_major_version = 0x9,
>> + .core_minor_version = 0x0,
>> .caps = &sm8550_dpu_caps,
>> .ubwc = &sm8550_ubwc_cfg,
>> .mdp_count = ARRAY_SIZE(sm8550_mdp),
>> 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 b860784ade72..8b900be3ea90 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> @@ -796,8 +796,9 @@ struct dpu_perf_cfg {
>> /**
>> * struct dpu_mdss_cfg - information of MDSS HW
>> * This is the main catalog data structure representing
>> - * this HW version. Contains number of instances,
>> - * register offsets, capabilities of the all MDSS HW sub-blocks.
>> + * this HW version. Contains dpu's major and minor versions,
>> + * number of instances, register offsets, capabilities of the
>> + * all MDSS HW sub-blocks.
>> *
>> * @dma_formats Supported formats for dma pipe
>> * @cursor_formats Supported formats for cursor pipe
>> @@ -805,6 +806,9 @@ struct dpu_perf_cfg {
>> * @mdss_irqs: Bitmap with the irqs supported by the target
>> */
>> struct dpu_mdss_cfg {
>> + u8 core_major_version;
>> + u8 core_minor_version;
>> +
>> const struct dpu_caps *caps;
>> const struct dpu_ubwc_cfg *ubwc;
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 1/3] drm/msm/dpu: re-introduce dpu core revision to the catalog
@ 2023-06-30 3:15 ` Abhinav Kumar
0 siblings, 0 replies; 26+ messages in thread
From: Abhinav Kumar @ 2023-06-30 3:15 UTC (permalink / raw)
To: Dmitry Baryshkov, freedreno, Rob Clark, Sean Paul,
Marijn Suijten, David Airlie, Daniel Vetter
Cc: quic_jesszhan, andersson, linux-kernel, dri-devel, linux-arm-msm
On 6/29/2023 5:13 PM, Dmitry Baryshkov wrote:
> On 29/06/2023 22:29, Abhinav Kumar 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;
>>
>> Since dpu's major and minor versions are now separate fields, lets
>> drop all the DPU_HW_VER macros.
>>
>> [1]: https://patchwork.freedesktop.org/patch/530891/?series=113910&rev=4
>>
>> changes in v3:
>> - drop DPU step version as features are not changing across steps
>> - add core_major_version / core_minor_version to avoid conflicts
>> - update the commit text to drop references to the dpu macros
>>
>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>> ---
>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h | 2 ++
>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h | 2 ++
>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h | 2 ++
>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h | 2 ++
>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h | 2 ++
>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h | 2 ++
>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h | 2 ++
>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_4_sm6350.h | 2 ++
>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h | 2 ++
>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_9_sm6375.h | 2 ++
>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h | 2 ++
>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h | 2 ++
>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h | 2 ++
>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h | 2 ++
>> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h | 2 ++
>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 8 ++++++--
>> 16 files changed, 36 insertions(+), 2 deletions(-)
>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>
> Nit: if there is a need for v-next, I'd probably suggest dropping th hex
> prefix from these values.
>
ack, I was debating in my head whether to keep it or not and kept it in
the end. I am fine to drop it too.
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
>> b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
>> index 7d0d0e74c3b0..a5d486783c3f 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
>> @@ -190,6 +190,8 @@ static const struct dpu_perf_cfg msm8998_perf_data
>> = {
>> };
>> const struct dpu_mdss_cfg dpu_msm8998_cfg = {
>> + .core_major_version = 0x3,
>> + .core_minor_version = 0x0,
>> .caps = &msm8998_dpu_caps,
>> .ubwc = &msm8998_ubwc_cfg,
>> .mdp_count = ARRAY_SIZE(msm8998_mdp),
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h
>> b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h
>> index b6098141bb9b..1fdb89a4b7a6 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h
>> @@ -194,6 +194,8 @@ static const struct dpu_perf_cfg sdm845_perf_data = {
>> };
>> const struct dpu_mdss_cfg dpu_sdm845_cfg = {
>> + .core_major_version = 0x4,
>> + .core_minor_version = 0x0,
>> .caps = &sdm845_dpu_caps,
>> .ubwc = &sdm845_ubwc_cfg,
>> .mdp_count = ARRAY_SIZE(sdm845_mdp),
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h
>> b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h
>> index b5f751354267..129c62cf450d 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h
>> @@ -208,6 +208,8 @@ static const struct dpu_perf_cfg sm8150_perf_data = {
>> };
>> const struct dpu_mdss_cfg dpu_sm8150_cfg = {
>> + .core_major_version = 0x5,
>> + .core_minor_version = 0x0,
>> .caps = &sm8150_dpu_caps,
>> .ubwc = &sm8150_ubwc_cfg,
>> .mdp_count = ARRAY_SIZE(sm8150_mdp),
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h
>> b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h
>> index 8ed2b263c5ea..ca037b070f44 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h
>> @@ -214,6 +214,8 @@ static const struct dpu_perf_cfg sc8180x_perf_data
>> = {
>> };
>> const struct dpu_mdss_cfg dpu_sc8180x_cfg = {
>> + .core_major_version = 0x5,
>> + .core_minor_version = 0x1,
>> .caps = &sc8180x_dpu_caps,
>> .ubwc = &sc8180x_ubwc_cfg,
>> .mdp_count = ARRAY_SIZE(sc8180x_mdp),
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h
>> b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h
>> index daebd2170041..e446af90767e 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h
>> @@ -214,6 +214,8 @@ static const struct dpu_perf_cfg sm8250_perf_data = {
>> };
>> const struct dpu_mdss_cfg dpu_sm8250_cfg = {
>> + .core_major_version = 0x6,
>> + .core_minor_version = 0x0,
>> .caps = &sm8250_dpu_caps,
>> .ubwc = &sm8250_ubwc_cfg,
>> .mdp_count = ARRAY_SIZE(sm8250_mdp),
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h
>> b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h
>> index 67566b07195a..88288c80b652 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h
>> @@ -132,6 +132,8 @@ static const struct dpu_perf_cfg sc7180_perf_data = {
>> };
>> const struct dpu_mdss_cfg dpu_sc7180_cfg = {
>> + .core_major_version = 0x6,
>> + .core_minor_version = 0x2,
>> .caps = &sc7180_dpu_caps,
>> .ubwc = &sc7180_ubwc_cfg,
>> .mdp_count = ARRAY_SIZE(sc7180_mdp),
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h
>> b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h
>> index 031fc8dae3c6..93c901502b5a 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h
>> @@ -102,6 +102,8 @@ static const struct dpu_perf_cfg sm6115_perf_data = {
>> };
>> const struct dpu_mdss_cfg dpu_sm6115_cfg = {
>> + .core_major_version = 0x6,
>> + .core_minor_version = 0x3,
>> .caps = &sm6115_dpu_caps,
>> .ubwc = &sm6115_ubwc_cfg,
>> .mdp_count = ARRAY_SIZE(sm6115_mdp),
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_4_sm6350.h
>> b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_4_sm6350.h
>> index 06eba23b0236..ff7e4b775fd5 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_4_sm6350.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_4_sm6350.h
>> @@ -141,6 +141,8 @@ static const struct dpu_perf_cfg sm6350_perf_data = {
>> };
>> const struct dpu_mdss_cfg dpu_sm6350_cfg = {
>> + .core_major_version = 0x6,
>> + .core_minor_version = 0x4,
>> .caps = &sm6350_dpu_caps,
>> .ubwc = &sm6350_ubwc_cfg,
>> .mdp_count = ARRAY_SIZE(sm6350_mdp),
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h
>> b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h
>> index f2808098af39..7bc86aa50e6f 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h
>> @@ -92,6 +92,8 @@ static const struct dpu_perf_cfg qcm2290_perf_data = {
>> };
>> const struct dpu_mdss_cfg dpu_qcm2290_cfg = {
>> + .core_major_version = 0x6,
>> + .core_minor_version = 0x5,
>> .caps = &qcm2290_dpu_caps,
>> .ubwc = &qcm2290_ubwc_cfg,
>> .mdp_count = ARRAY_SIZE(qcm2290_mdp),
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_9_sm6375.h
>> b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_9_sm6375.h
>> index 241fa6746674..d92890f090d4 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_9_sm6375.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_6_9_sm6375.h
>> @@ -107,6 +107,8 @@ static const struct dpu_perf_cfg sm6375_perf_data = {
>> };
>> const struct dpu_mdss_cfg dpu_sm6375_cfg = {
>> + .core_major_version = 0x6,
>> + .core_minor_version = 0x9,
>> .caps = &sm6375_dpu_caps,
>> .ubwc = &sm6375_ubwc_cfg,
>> .mdp_count = ARRAY_SIZE(sm6375_mdp),
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
>> b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
>> index 8da424eaee6a..8a2dc56c79f8 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h
>> @@ -213,6 +213,8 @@ static const struct dpu_perf_cfg sm8350_perf_data = {
>> };
>> const struct dpu_mdss_cfg dpu_sm8350_cfg = {
>> + .core_major_version = 0x7,
>> + .core_minor_version = 0x0,
>> .caps = &sm8350_dpu_caps,
>> .ubwc = &sm8350_ubwc_cfg,
>> .mdp_count = ARRAY_SIZE(sm8350_mdp),
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
>> b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
>> index 900fee410e11..bba7bdb9bd42 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h
>> @@ -154,6 +154,8 @@ static const struct dpu_perf_cfg sc7280_perf_data = {
>> };
>> const struct dpu_mdss_cfg dpu_sc7280_cfg = {
>> + .core_major_version = 0x7,
>> + .core_minor_version = 0x2,
>> .caps = &sc7280_dpu_caps,
>> .ubwc = &sc7280_ubwc_cfg,
>> .mdp_count = ARRAY_SIZE(sc7280_mdp),
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
>> b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
>> index f6ce6b090f71..3f51b802b6b7 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h
>> @@ -217,6 +217,8 @@ static const struct dpu_perf_cfg
>> sc8280xp_perf_data = {
>> };
>> const struct dpu_mdss_cfg dpu_sc8280xp_cfg = {
>> + .core_major_version = 0x8,
>> + .core_minor_version = 0x0,
>> .caps = &sc8280xp_dpu_caps,
>> .ubwc = &sc8280xp_ubwc_cfg,
>> .mdp_count = ARRAY_SIZE(sc8280xp_mdp),
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
>> b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
>> index 8d13c369213c..20acff9db979 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h
>> @@ -221,6 +221,8 @@ static const struct dpu_perf_cfg sm8450_perf_data = {
>> };
>> const struct dpu_mdss_cfg dpu_sm8450_cfg = {
>> + .core_major_version = 0x8,
>> + .core_minor_version = 0x1,
>> .caps = &sm8450_dpu_caps,
>> .ubwc = &sm8450_ubwc_cfg,
>> .mdp_count = ARRAY_SIZE(sm8450_mdp),
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
>> b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
>> index f17b9a7fee85..89fdf334a0aa 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h
>> @@ -225,6 +225,8 @@ static const struct dpu_perf_cfg sm8550_perf_data = {
>> };
>> const struct dpu_mdss_cfg dpu_sm8550_cfg = {
>> + .core_major_version = 0x9,
>> + .core_minor_version = 0x0,
>> .caps = &sm8550_dpu_caps,
>> .ubwc = &sm8550_ubwc_cfg,
>> .mdp_count = ARRAY_SIZE(sm8550_mdp),
>> 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 b860784ade72..8b900be3ea90 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> @@ -796,8 +796,9 @@ struct dpu_perf_cfg {
>> /**
>> * struct dpu_mdss_cfg - information of MDSS HW
>> * This is the main catalog data structure representing
>> - * this HW version. Contains number of instances,
>> - * register offsets, capabilities of the all MDSS HW sub-blocks.
>> + * this HW version. Contains dpu's major and minor versions,
>> + * number of instances, register offsets, capabilities of the
>> + * all MDSS HW sub-blocks.
>> *
>> * @dma_formats Supported formats for dma pipe
>> * @cursor_formats Supported formats for cursor pipe
>> @@ -805,6 +806,9 @@ struct dpu_perf_cfg {
>> * @mdss_irqs: Bitmap with the irqs supported by the target
>> */
>> struct dpu_mdss_cfg {
>> + u8 core_major_version;
>> + u8 core_minor_version;
>> +
>> const struct dpu_caps *caps;
>> const struct dpu_ubwc_cfg *ubwc;
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 1/3] drm/msm/dpu: re-introduce dpu core revision to the catalog
2023-06-30 0:24 ` Dmitry Baryshkov
@ 2023-06-30 3:17 ` Abhinav Kumar
-1 siblings, 0 replies; 26+ messages in thread
From: Abhinav Kumar @ 2023-06-30 3:17 UTC (permalink / raw)
To: Dmitry Baryshkov, freedreno, Rob Clark, Sean Paul,
Marijn Suijten, David Airlie, Daniel Vetter
Cc: quic_jesszhan, andersson, linux-kernel, dri-devel, linux-arm-msm
On 6/29/2023 5:24 PM, Dmitry Baryshkov wrote:
> On 29/06/2023 22:29, Abhinav Kumar 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.
>
> Generic note: this description can be moved to the cover letter, it
> covers the series intent.
>
I kept it here as I didnt really have a cover letter but I can add one
and move this there.
>> 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;
>>
>> Since dpu's major and minor versions are now separate fields, lets
>> drop all the DPU_HW_VER macros.
>>
>> [1]: https://patchwork.freedesktop.org/patch/530891/?series=113910&rev=4
>
> Please use `commit aabbcc ("do this and that")' in the commit messages.
>
Ack.
>>
>> changes in v3:
>> - drop DPU step version as features are not changing across steps
>> - add core_major_version / core_minor_version to avoid conflicts
>> - update the commit text to drop references to the dpu macros
>>
>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 1/3] drm/msm/dpu: re-introduce dpu core revision to the catalog
@ 2023-06-30 3:17 ` Abhinav Kumar
0 siblings, 0 replies; 26+ messages in thread
From: Abhinav Kumar @ 2023-06-30 3:17 UTC (permalink / raw)
To: Dmitry Baryshkov, freedreno, Rob Clark, Sean Paul,
Marijn Suijten, David Airlie, Daniel Vetter
Cc: dri-devel, quic_jesszhan, andersson, linux-arm-msm, linux-kernel
On 6/29/2023 5:24 PM, Dmitry Baryshkov wrote:
> On 29/06/2023 22:29, Abhinav Kumar 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.
>
> Generic note: this description can be moved to the cover letter, it
> covers the series intent.
>
I kept it here as I didnt really have a cover letter but I can add one
and move this there.
>> 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;
>>
>> Since dpu's major and minor versions are now separate fields, lets
>> drop all the DPU_HW_VER macros.
>>
>> [1]: https://patchwork.freedesktop.org/patch/530891/?series=113910&rev=4
>
> Please use `commit aabbcc ("do this and that")' in the commit messages.
>
Ack.
>>
>> changes in v3:
>> - drop DPU step version as features are not changing across steps
>> - add core_major_version / core_minor_version to avoid conflicts
>> - update the commit text to drop references to the dpu macros
>>
>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 1/3] drm/msm/dpu: re-introduce dpu core revision to the catalog
2023-06-30 3:17 ` Abhinav Kumar
@ 2023-06-30 3:19 ` Dmitry Baryshkov
-1 siblings, 0 replies; 26+ messages in thread
From: Dmitry Baryshkov @ 2023-06-30 3:19 UTC (permalink / raw)
To: Abhinav Kumar, freedreno, Rob Clark, Sean Paul, Marijn Suijten,
David Airlie, Daniel Vetter
Cc: quic_jesszhan, andersson, linux-kernel, dri-devel, linux-arm-msm
On 30/06/2023 06:17, Abhinav Kumar wrote:
>
>
> On 6/29/2023 5:24 PM, Dmitry Baryshkov wrote:
>> On 29/06/2023 22:29, Abhinav Kumar 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.
>>
>> Generic note: this description can be moved to the cover letter, it
>> covers the series intent.
>>
>
> I kept it here as I didnt really have a cover letter but I can add one
> and move this there.
Yes, please. I suppose that any series of more than a single non-trivial
patch should have a cover letter, which describes the intentions and the
ideas behind the series.
>
>>> 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;
>>>
>>> Since dpu's major and minor versions are now separate fields, lets
>>> drop all the DPU_HW_VER macros.
>>>
>>> [1]: https://patchwork.freedesktop.org/patch/530891/?series=113910&rev=4
>>
>> Please use `commit aabbcc ("do this and that")' in the commit messages.
>>
>
> Ack.
>
>>>
>>> changes in v3:
>>> - drop DPU step version as features are not changing across steps
>>> - add core_major_version / core_minor_version to avoid conflicts
>>> - update the commit text to drop references to the dpu macros
>>>
>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 1/3] drm/msm/dpu: re-introduce dpu core revision to the catalog
@ 2023-06-30 3:19 ` Dmitry Baryshkov
0 siblings, 0 replies; 26+ messages in thread
From: Dmitry Baryshkov @ 2023-06-30 3:19 UTC (permalink / raw)
To: Abhinav Kumar, freedreno, Rob Clark, Sean Paul, Marijn Suijten,
David Airlie, Daniel Vetter
Cc: dri-devel, quic_jesszhan, andersson, linux-arm-msm, linux-kernel
On 30/06/2023 06:17, Abhinav Kumar wrote:
>
>
> On 6/29/2023 5:24 PM, Dmitry Baryshkov wrote:
>> On 29/06/2023 22:29, Abhinav Kumar 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.
>>
>> Generic note: this description can be moved to the cover letter, it
>> covers the series intent.
>>
>
> I kept it here as I didnt really have a cover letter but I can add one
> and move this there.
Yes, please. I suppose that any series of more than a single non-trivial
patch should have a cover letter, which describes the intentions and the
ideas behind the series.
>
>>> 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;
>>>
>>> Since dpu's major and minor versions are now separate fields, lets
>>> drop all the DPU_HW_VER macros.
>>>
>>> [1]: https://patchwork.freedesktop.org/patch/530891/?series=113910&rev=4
>>
>> Please use `commit aabbcc ("do this and that")' in the commit messages.
>>
>
> Ack.
>
>>>
>>> changes in v3:
>>> - drop DPU step version as features are not changing across steps
>>> - add core_major_version / core_minor_version to avoid conflicts
>>> - update the commit text to drop references to the dpu macros
>>>
>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 2/3] drm/msm/dpu: use dpu core's major version to enable data compress
2023-06-30 3:07 ` Abhinav Kumar
@ 2023-06-30 3:22 ` Dmitry Baryshkov
-1 siblings, 0 replies; 26+ messages in thread
From: Dmitry Baryshkov @ 2023-06-30 3:22 UTC (permalink / raw)
To: Abhinav Kumar, freedreno, Rob Clark, Sean Paul, Marijn Suijten,
David Airlie, Daniel Vetter
Cc: dri-devel, quic_jesszhan, andersson, linux-arm-msm, linux-kernel
On 30/06/2023 06:07, Abhinav Kumar wrote:
>
>
> On 6/29/2023 5:20 PM, Dmitry Baryshkov wrote:
>> On 29/06/2023 22:29, Abhinav Kumar wrote:
>>> Instead of using a feature bit to decide whether to enable data
>>> compress or not for DSC use-cases, use dpu core's major version instead.
>>> This will avoid defining feature bits for every bit level details of
>>> registers.
>>>
>>> Also, rename the intf's enable_compression() op to program_datapath()
>>> and allow it to accept a struct intf_dpu_datapath_cfg to program
>>> all the bits at once. This can be re-used by widebus later on as
>>> well as it touches the same register.
>>
>> Two separate commits please, because...
>>
>
> I thought of it but it seemed better together and was the only way I
> could think of. Please see below.
>
>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>> ---
>>> .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 9 +++++++--
>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 9 +++++----
>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 16 ++++++++++++++--
>>> 3 files changed, 26 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>> index b856c6286c85..f4e15b4c4cc9 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>> @@ -50,6 +50,7 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
>>> to_dpu_encoder_phys_cmd(phys_enc);
>>> struct dpu_hw_ctl *ctl;
>>> struct dpu_hw_intf_cfg intf_cfg = { 0 };
>>> + struct dpu_kms *dpu_kms = phys_enc->dpu_kms;
>>> ctl = phys_enc->hw_ctl;
>>> if (!ctl->ops.setup_intf_cfg)
>>> @@ -68,8 +69,12 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
>>> phys_enc->hw_intf,
>>> phys_enc->hw_pp->idx);
>>> - if (intf_cfg.dsc != 0 && phys_enc->hw_intf->ops.enable_compression)
>>> - phys_enc->hw_intf->ops.enable_compression(phys_enc->hw_intf);
>>> + if (intf_cfg.dsc != 0 && dpu_kms->catalog->core_major_version >=
>>> 0x7) {
>>
>> ... because this becomes incorrect. The datapath should be programmed
>> in all the cases, if there is a corresponding callback. intf_cfg.dsc
>> should be used as a condition to set datapath_cfg.
>>
>
> The issue is that today we do not have dpu_mdss_cfg as part of
> dpu_hw_intf nor _setup_intf_ops because all of those have been dropped
> with some rework or cleanup.
Pass dpu_mdss_cfg to dpu_hw_intf_init(). It was removed as a cleanup,
now we can reintroduce it.
>
> Ideally even I would like to assign this op only for core_rev >=7 but
> that information is no longer available. We would have to start passing
> the major and minor versions to _setup_intf_ops() to go with that
> approach. So without making all of those changes, the only way I had was
> to assign the op unconditionally but call it only for major_rev >= 7.
>
> Passing core_rev to the op itself so that we can write the register only
> for core_rev >=7 is an option but then what if some bits start becoming
> usable only after minor rev. then we will have to start passing major
> and minor rev to program_datapath too. Again getting little messy.
>
> I am open to ideas to achieve the goal of assigning this op only for
> core_rev >=7 other than what I wrote above.
>
>>
>>> + struct intf_dpu_datapath_cfg datapath_cfg = { 0 };
>>
>> No need for `0' in the init, empty braces would be enough.
>>
>
> ack.
>
>>> +
>>> + datapath_cfg.data_compress = true;
>>> + phys_enc->hw_intf->ops.program_datapath(phys_enc->hw_intf,
>>> &datapath_cfg);
>>> + }
>>> }
>>> static void dpu_encoder_phys_cmd_pp_tx_done_irq(void *arg, int
>>> irq_idx)
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>> index 5b0f6627e29b..85333df08fbc 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>> @@ -513,11 +513,13 @@ static void
>>> dpu_hw_intf_disable_autorefresh(struct dpu_hw_intf *intf,
>>> }
>>> -static void dpu_hw_intf_enable_compression(struct dpu_hw_intf *ctx)
>>> +static void dpu_hw_intf_program_datapath(struct dpu_hw_intf *ctx,
>>> + struct intf_dpu_datapath_cfg *datapath_cfg)
>>> {
>>> u32 intf_cfg2 = DPU_REG_READ(&ctx->hw, INTF_CONFIG2);
>>> - intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
>>> + if (datapath_cfg->data_compress)
>>> + intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
>>> DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, intf_cfg2);
>>> }
>>> @@ -543,8 +545,7 @@ static void _setup_intf_ops(struct
>>> dpu_hw_intf_ops *ops,
>>> ops->disable_autorefresh = dpu_hw_intf_disable_autorefresh;
>>> }
>>> - if (cap & BIT(DPU_INTF_DATA_COMPRESS))
>>> - ops->enable_compression = dpu_hw_intf_enable_compression;
>>> + ops->program_datapath = dpu_hw_intf_program_datapath;
>>
>> The `core_major_version >= 7' should either be here or in the callback
>> itself.
>>
>
> Yes, ideally I would like it like that but please see above why I
> couldnt do it.
>
>>> }
>>> struct dpu_hw_intf *dpu_hw_intf_init(const struct dpu_intf_cfg *cfg,
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>>> index 99e21c4137f9..f736dca38463 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>>> @@ -48,6 +48,11 @@ struct intf_status {
>>> u32 line_count; /* current line count including blanking */
>>> };
>>> +struct intf_dpu_datapath_cfg {
>>> + u8 data_compress; /* enable data compress between dpu and dsi */
>>> + /* can be expanded for other programmable bits */
>>> +};
>>
>> I'd say, dpu_datapath is too generic. What about intf_cmd_mode_cfg?
>>
>
> The goal was to keep it generic. Its actually the handshake between DPU
> and interface datapath so I chose that name.
Do you have plans of using it for the video mode?
>
> This is not specific to command mode and intf_cfg is already there so I
> chose that one :)
>
>>> +
>>> /**
>>> * struct dpu_hw_intf_ops : Interface to the interface Hw driver
>>> functions
>>> * Assumption is these functions will be called after clocks are
>>> enabled
>>> @@ -70,7 +75,7 @@ struct intf_status {
>>> * @get_autorefresh: Retrieve autorefresh config from
>>> hardware
>>> * Return: 0 on success, -ETIMEDOUT on
>>> timeout
>>> * @vsync_sel: Select vsync signal for tear-effect
>>> configuration
>>> - * @enable_compression: Enable data compression
>>> + * @program_datapath: Program the DPU to interface
>>> datapath for relevant chipsets
>>> */
>>> struct dpu_hw_intf_ops {
>>> void (*setup_timing_gen)(struct dpu_hw_intf *intf,
>>> @@ -108,7 +113,14 @@ struct dpu_hw_intf_ops {
>>> */
>>> void (*disable_autorefresh)(struct dpu_hw_intf *intf, uint32_t
>>> encoder_id, u16 vdisplay);
>>> - void (*enable_compression)(struct dpu_hw_intf *intf);
>>> + /**
>>> + * Program the DPU to intf datapath by specifying
>>> + * which of the settings need to be programmed for
>>> + * use-cases which need DPU-intf handshake such as
>>> + * widebus, compression etc.
>>
>> This is not a valid kerneldoc.
>>
>
> hmmm ... ok so just // ?
>
> I referred disable_autorefresh from above and did the same.
>
>>> + */
>>> + void (*program_datapath)(struct dpu_hw_intf *intf,
>>> + struct intf_dpu_datapath_cfg *datapath_cfg);
>>> };
>>> struct dpu_hw_intf {
>>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 2/3] drm/msm/dpu: use dpu core's major version to enable data compress
@ 2023-06-30 3:22 ` Dmitry Baryshkov
0 siblings, 0 replies; 26+ messages in thread
From: Dmitry Baryshkov @ 2023-06-30 3:22 UTC (permalink / raw)
To: Abhinav Kumar, freedreno, Rob Clark, Sean Paul, Marijn Suijten,
David Airlie, Daniel Vetter
Cc: quic_jesszhan, andersson, linux-kernel, dri-devel, linux-arm-msm
On 30/06/2023 06:07, Abhinav Kumar wrote:
>
>
> On 6/29/2023 5:20 PM, Dmitry Baryshkov wrote:
>> On 29/06/2023 22:29, Abhinav Kumar wrote:
>>> Instead of using a feature bit to decide whether to enable data
>>> compress or not for DSC use-cases, use dpu core's major version instead.
>>> This will avoid defining feature bits for every bit level details of
>>> registers.
>>>
>>> Also, rename the intf's enable_compression() op to program_datapath()
>>> and allow it to accept a struct intf_dpu_datapath_cfg to program
>>> all the bits at once. This can be re-used by widebus later on as
>>> well as it touches the same register.
>>
>> Two separate commits please, because...
>>
>
> I thought of it but it seemed better together and was the only way I
> could think of. Please see below.
>
>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>> ---
>>> .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 9 +++++++--
>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 9 +++++----
>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 16 ++++++++++++++--
>>> 3 files changed, 26 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>> index b856c6286c85..f4e15b4c4cc9 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>> @@ -50,6 +50,7 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
>>> to_dpu_encoder_phys_cmd(phys_enc);
>>> struct dpu_hw_ctl *ctl;
>>> struct dpu_hw_intf_cfg intf_cfg = { 0 };
>>> + struct dpu_kms *dpu_kms = phys_enc->dpu_kms;
>>> ctl = phys_enc->hw_ctl;
>>> if (!ctl->ops.setup_intf_cfg)
>>> @@ -68,8 +69,12 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
>>> phys_enc->hw_intf,
>>> phys_enc->hw_pp->idx);
>>> - if (intf_cfg.dsc != 0 && phys_enc->hw_intf->ops.enable_compression)
>>> - phys_enc->hw_intf->ops.enable_compression(phys_enc->hw_intf);
>>> + if (intf_cfg.dsc != 0 && dpu_kms->catalog->core_major_version >=
>>> 0x7) {
>>
>> ... because this becomes incorrect. The datapath should be programmed
>> in all the cases, if there is a corresponding callback. intf_cfg.dsc
>> should be used as a condition to set datapath_cfg.
>>
>
> The issue is that today we do not have dpu_mdss_cfg as part of
> dpu_hw_intf nor _setup_intf_ops because all of those have been dropped
> with some rework or cleanup.
Pass dpu_mdss_cfg to dpu_hw_intf_init(). It was removed as a cleanup,
now we can reintroduce it.
>
> Ideally even I would like to assign this op only for core_rev >=7 but
> that information is no longer available. We would have to start passing
> the major and minor versions to _setup_intf_ops() to go with that
> approach. So without making all of those changes, the only way I had was
> to assign the op unconditionally but call it only for major_rev >= 7.
>
> Passing core_rev to the op itself so that we can write the register only
> for core_rev >=7 is an option but then what if some bits start becoming
> usable only after minor rev. then we will have to start passing major
> and minor rev to program_datapath too. Again getting little messy.
>
> I am open to ideas to achieve the goal of assigning this op only for
> core_rev >=7 other than what I wrote above.
>
>>
>>> + struct intf_dpu_datapath_cfg datapath_cfg = { 0 };
>>
>> No need for `0' in the init, empty braces would be enough.
>>
>
> ack.
>
>>> +
>>> + datapath_cfg.data_compress = true;
>>> + phys_enc->hw_intf->ops.program_datapath(phys_enc->hw_intf,
>>> &datapath_cfg);
>>> + }
>>> }
>>> static void dpu_encoder_phys_cmd_pp_tx_done_irq(void *arg, int
>>> irq_idx)
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>> index 5b0f6627e29b..85333df08fbc 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>> @@ -513,11 +513,13 @@ static void
>>> dpu_hw_intf_disable_autorefresh(struct dpu_hw_intf *intf,
>>> }
>>> -static void dpu_hw_intf_enable_compression(struct dpu_hw_intf *ctx)
>>> +static void dpu_hw_intf_program_datapath(struct dpu_hw_intf *ctx,
>>> + struct intf_dpu_datapath_cfg *datapath_cfg)
>>> {
>>> u32 intf_cfg2 = DPU_REG_READ(&ctx->hw, INTF_CONFIG2);
>>> - intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
>>> + if (datapath_cfg->data_compress)
>>> + intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
>>> DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, intf_cfg2);
>>> }
>>> @@ -543,8 +545,7 @@ static void _setup_intf_ops(struct
>>> dpu_hw_intf_ops *ops,
>>> ops->disable_autorefresh = dpu_hw_intf_disable_autorefresh;
>>> }
>>> - if (cap & BIT(DPU_INTF_DATA_COMPRESS))
>>> - ops->enable_compression = dpu_hw_intf_enable_compression;
>>> + ops->program_datapath = dpu_hw_intf_program_datapath;
>>
>> The `core_major_version >= 7' should either be here or in the callback
>> itself.
>>
>
> Yes, ideally I would like it like that but please see above why I
> couldnt do it.
>
>>> }
>>> struct dpu_hw_intf *dpu_hw_intf_init(const struct dpu_intf_cfg *cfg,
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>>> index 99e21c4137f9..f736dca38463 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>>> @@ -48,6 +48,11 @@ struct intf_status {
>>> u32 line_count; /* current line count including blanking */
>>> };
>>> +struct intf_dpu_datapath_cfg {
>>> + u8 data_compress; /* enable data compress between dpu and dsi */
>>> + /* can be expanded for other programmable bits */
>>> +};
>>
>> I'd say, dpu_datapath is too generic. What about intf_cmd_mode_cfg?
>>
>
> The goal was to keep it generic. Its actually the handshake between DPU
> and interface datapath so I chose that name.
Do you have plans of using it for the video mode?
>
> This is not specific to command mode and intf_cfg is already there so I
> chose that one :)
>
>>> +
>>> /**
>>> * struct dpu_hw_intf_ops : Interface to the interface Hw driver
>>> functions
>>> * Assumption is these functions will be called after clocks are
>>> enabled
>>> @@ -70,7 +75,7 @@ struct intf_status {
>>> * @get_autorefresh: Retrieve autorefresh config from
>>> hardware
>>> * Return: 0 on success, -ETIMEDOUT on
>>> timeout
>>> * @vsync_sel: Select vsync signal for tear-effect
>>> configuration
>>> - * @enable_compression: Enable data compression
>>> + * @program_datapath: Program the DPU to interface
>>> datapath for relevant chipsets
>>> */
>>> struct dpu_hw_intf_ops {
>>> void (*setup_timing_gen)(struct dpu_hw_intf *intf,
>>> @@ -108,7 +113,14 @@ struct dpu_hw_intf_ops {
>>> */
>>> void (*disable_autorefresh)(struct dpu_hw_intf *intf, uint32_t
>>> encoder_id, u16 vdisplay);
>>> - void (*enable_compression)(struct dpu_hw_intf *intf);
>>> + /**
>>> + * Program the DPU to intf datapath by specifying
>>> + * which of the settings need to be programmed for
>>> + * use-cases which need DPU-intf handshake such as
>>> + * widebus, compression etc.
>>
>> This is not a valid kerneldoc.
>>
>
> hmmm ... ok so just // ?
>
> I referred disable_autorefresh from above and did the same.
>
>>> + */
>>> + void (*program_datapath)(struct dpu_hw_intf *intf,
>>> + struct intf_dpu_datapath_cfg *datapath_cfg);
>>> };
>>> struct dpu_hw_intf {
>>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 2/3] drm/msm/dpu: use dpu core's major version to enable data compress
2023-06-30 3:22 ` Dmitry Baryshkov
@ 2023-06-30 3:30 ` Abhinav Kumar
-1 siblings, 0 replies; 26+ messages in thread
From: Abhinav Kumar @ 2023-06-30 3:30 UTC (permalink / raw)
To: Dmitry Baryshkov, freedreno, Rob Clark, Sean Paul,
Marijn Suijten, David Airlie, Daniel Vetter
Cc: dri-devel, quic_jesszhan, andersson, linux-arm-msm, linux-kernel
On 6/29/2023 8:22 PM, Dmitry Baryshkov wrote:
> On 30/06/2023 06:07, Abhinav Kumar wrote:
>>
>>
>> On 6/29/2023 5:20 PM, Dmitry Baryshkov wrote:
>>> On 29/06/2023 22:29, Abhinav Kumar wrote:
>>>> Instead of using a feature bit to decide whether to enable data
>>>> compress or not for DSC use-cases, use dpu core's major version
>>>> instead.
>>>> This will avoid defining feature bits for every bit level details of
>>>> registers.
>>>>
>>>> Also, rename the intf's enable_compression() op to program_datapath()
>>>> and allow it to accept a struct intf_dpu_datapath_cfg to program
>>>> all the bits at once. This can be re-used by widebus later on as
>>>> well as it touches the same register.
>>>
>>> Two separate commits please, because...
>>>
>>
>> I thought of it but it seemed better together and was the only way I
>> could think of. Please see below.
>>
>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>>> ---
>>>> .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 9 +++++++--
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 9 +++++----
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 16
>>>> ++++++++++++++--
>>>> 3 files changed, 26 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>>> index b856c6286c85..f4e15b4c4cc9 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>>> @@ -50,6 +50,7 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
>>>> to_dpu_encoder_phys_cmd(phys_enc);
>>>> struct dpu_hw_ctl *ctl;
>>>> struct dpu_hw_intf_cfg intf_cfg = { 0 };
>>>> + struct dpu_kms *dpu_kms = phys_enc->dpu_kms;
>>>> ctl = phys_enc->hw_ctl;
>>>> if (!ctl->ops.setup_intf_cfg)
>>>> @@ -68,8 +69,12 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
>>>> phys_enc->hw_intf,
>>>> phys_enc->hw_pp->idx);
>>>> - if (intf_cfg.dsc != 0 &&
>>>> phys_enc->hw_intf->ops.enable_compression)
>>>> - phys_enc->hw_intf->ops.enable_compression(phys_enc->hw_intf);
>>>> + if (intf_cfg.dsc != 0 && dpu_kms->catalog->core_major_version
>>>> >= 0x7) {
>>>
>>> ... because this becomes incorrect. The datapath should be programmed
>>> in all the cases, if there is a corresponding callback. intf_cfg.dsc
>>> should be used as a condition to set datapath_cfg.
>>>
>>
>> The issue is that today we do not have dpu_mdss_cfg as part of
>> dpu_hw_intf nor _setup_intf_ops because all of those have been dropped
>> with some rework or cleanup.
>
> Pass dpu_mdss_cfg to dpu_hw_intf_init(). It was removed as a cleanup,
> now we can reintroduce it.
>
Thanks, that will address all these concerns.
I wanted to get agreement before re-introducing it and also make sure
there was no other way.
>>
>> Ideally even I would like to assign this op only for core_rev >=7 but
>> that information is no longer available. We would have to start
>> passing the major and minor versions to _setup_intf_ops() to go with
>> that approach. So without making all of those changes, the only way I
>> had was to assign the op unconditionally but call it only for
>> major_rev >= 7.
>>
>> Passing core_rev to the op itself so that we can write the register
>> only for core_rev >=7 is an option but then what if some bits start
>> becoming usable only after minor rev. then we will have to start
>> passing major and minor rev to program_datapath too. Again getting
>> little messy.
>>
>> I am open to ideas to achieve the goal of assigning this op only for
>> core_rev >=7 other than what I wrote above.
>>
>>>
>>>> + struct intf_dpu_datapath_cfg datapath_cfg = { 0 };
>>>
>>> No need for `0' in the init, empty braces would be enough.
>>>
>>
>> ack.
>>
>>>> +
>>>> + datapath_cfg.data_compress = true;
>>>> + phys_enc->hw_intf->ops.program_datapath(phys_enc->hw_intf,
>>>> &datapath_cfg);
>>>> + }
>>>> }
>>>> static void dpu_encoder_phys_cmd_pp_tx_done_irq(void *arg, int
>>>> irq_idx)
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>>> index 5b0f6627e29b..85333df08fbc 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>>> @@ -513,11 +513,13 @@ static void
>>>> dpu_hw_intf_disable_autorefresh(struct dpu_hw_intf *intf,
>>>> }
>>>> -static void dpu_hw_intf_enable_compression(struct dpu_hw_intf *ctx)
>>>> +static void dpu_hw_intf_program_datapath(struct dpu_hw_intf *ctx,
>>>> + struct intf_dpu_datapath_cfg *datapath_cfg)
>>>> {
>>>> u32 intf_cfg2 = DPU_REG_READ(&ctx->hw, INTF_CONFIG2);
>>>> - intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
>>>> + if (datapath_cfg->data_compress)
>>>> + intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
>>>> DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, intf_cfg2);
>>>> }
>>>> @@ -543,8 +545,7 @@ static void _setup_intf_ops(struct
>>>> dpu_hw_intf_ops *ops,
>>>> ops->disable_autorefresh = dpu_hw_intf_disable_autorefresh;
>>>> }
>>>> - if (cap & BIT(DPU_INTF_DATA_COMPRESS))
>>>> - ops->enable_compression = dpu_hw_intf_enable_compression;
>>>> + ops->program_datapath = dpu_hw_intf_program_datapath;
>>>
>>> The `core_major_version >= 7' should either be here or in the
>>> callback itself.
>>>
>>
>> Yes, ideally I would like it like that but please see above why I
>> couldnt do it.
>>
>>>> }
>>>> struct dpu_hw_intf *dpu_hw_intf_init(const struct dpu_intf_cfg *cfg,
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>>>> index 99e21c4137f9..f736dca38463 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>>>> @@ -48,6 +48,11 @@ struct intf_status {
>>>> u32 line_count; /* current line count including
>>>> blanking */
>>>> };
>>>> +struct intf_dpu_datapath_cfg {
>>>> + u8 data_compress; /* enable data compress between dpu and
>>>> dsi */
>>>> + /* can be expanded for other programmable bits */
>>>> +};
>>>
>>> I'd say, dpu_datapath is too generic. What about intf_cmd_mode_cfg?
>>>
>>
>> The goal was to keep it generic. Its actually the handshake between
>> DPU and interface datapath so I chose that name.
>
> Do you have plans of using it for the video mode?
>
No because we didnt want to touch the video mode path as that was
discussed in the widebus series already.
Ok, I am fine with intf_cmd_mode_cfg in that case.
>>
>> This is not specific to command mode and intf_cfg is already there so
>> I chose that one :)
>>
>>>> +
>>>> /**
>>>> * struct dpu_hw_intf_ops : Interface to the interface Hw driver
>>>> functions
>>>> * Assumption is these functions will be called after clocks are
>>>> enabled
>>>> @@ -70,7 +75,7 @@ struct intf_status {
>>>> * @get_autorefresh: Retrieve autorefresh config from
>>>> hardware
>>>> * Return: 0 on success, -ETIMEDOUT
>>>> on timeout
>>>> * @vsync_sel: Select vsync signal for
>>>> tear-effect configuration
>>>> - * @enable_compression: Enable data compression
>>>> + * @program_datapath: Program the DPU to interface
>>>> datapath for relevant chipsets
>>>> */
>>>> struct dpu_hw_intf_ops {
>>>> void (*setup_timing_gen)(struct dpu_hw_intf *intf,
>>>> @@ -108,7 +113,14 @@ struct dpu_hw_intf_ops {
>>>> */
>>>> void (*disable_autorefresh)(struct dpu_hw_intf *intf, uint32_t
>>>> encoder_id, u16 vdisplay);
>>>> - void (*enable_compression)(struct dpu_hw_intf *intf);
>>>> + /**
>>>> + * Program the DPU to intf datapath by specifying
>>>> + * which of the settings need to be programmed for
>>>> + * use-cases which need DPU-intf handshake such as
>>>> + * widebus, compression etc.
>>>
>>> This is not a valid kerneldoc.
>>>
>>
>> hmmm ... ok so just // ?
>>
>> I referred disable_autorefresh from above and did the same.
>>
>>>> + */
>>>> + void (*program_datapath)(struct dpu_hw_intf *intf,
>>>> + struct intf_dpu_datapath_cfg *datapath_cfg);
>>>> };
>>>> struct dpu_hw_intf {
>>>
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 2/3] drm/msm/dpu: use dpu core's major version to enable data compress
@ 2023-06-30 3:30 ` Abhinav Kumar
0 siblings, 0 replies; 26+ messages in thread
From: Abhinav Kumar @ 2023-06-30 3:30 UTC (permalink / raw)
To: Dmitry Baryshkov, freedreno, Rob Clark, Sean Paul,
Marijn Suijten, David Airlie, Daniel Vetter
Cc: quic_jesszhan, andersson, linux-kernel, dri-devel, linux-arm-msm
On 6/29/2023 8:22 PM, Dmitry Baryshkov wrote:
> On 30/06/2023 06:07, Abhinav Kumar wrote:
>>
>>
>> On 6/29/2023 5:20 PM, Dmitry Baryshkov wrote:
>>> On 29/06/2023 22:29, Abhinav Kumar wrote:
>>>> Instead of using a feature bit to decide whether to enable data
>>>> compress or not for DSC use-cases, use dpu core's major version
>>>> instead.
>>>> This will avoid defining feature bits for every bit level details of
>>>> registers.
>>>>
>>>> Also, rename the intf's enable_compression() op to program_datapath()
>>>> and allow it to accept a struct intf_dpu_datapath_cfg to program
>>>> all the bits at once. This can be re-used by widebus later on as
>>>> well as it touches the same register.
>>>
>>> Two separate commits please, because...
>>>
>>
>> I thought of it but it seemed better together and was the only way I
>> could think of. Please see below.
>>
>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>>> ---
>>>> .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 9 +++++++--
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 9 +++++----
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 16
>>>> ++++++++++++++--
>>>> 3 files changed, 26 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>>> index b856c6286c85..f4e15b4c4cc9 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>>> @@ -50,6 +50,7 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
>>>> to_dpu_encoder_phys_cmd(phys_enc);
>>>> struct dpu_hw_ctl *ctl;
>>>> struct dpu_hw_intf_cfg intf_cfg = { 0 };
>>>> + struct dpu_kms *dpu_kms = phys_enc->dpu_kms;
>>>> ctl = phys_enc->hw_ctl;
>>>> if (!ctl->ops.setup_intf_cfg)
>>>> @@ -68,8 +69,12 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
>>>> phys_enc->hw_intf,
>>>> phys_enc->hw_pp->idx);
>>>> - if (intf_cfg.dsc != 0 &&
>>>> phys_enc->hw_intf->ops.enable_compression)
>>>> - phys_enc->hw_intf->ops.enable_compression(phys_enc->hw_intf);
>>>> + if (intf_cfg.dsc != 0 && dpu_kms->catalog->core_major_version
>>>> >= 0x7) {
>>>
>>> ... because this becomes incorrect. The datapath should be programmed
>>> in all the cases, if there is a corresponding callback. intf_cfg.dsc
>>> should be used as a condition to set datapath_cfg.
>>>
>>
>> The issue is that today we do not have dpu_mdss_cfg as part of
>> dpu_hw_intf nor _setup_intf_ops because all of those have been dropped
>> with some rework or cleanup.
>
> Pass dpu_mdss_cfg to dpu_hw_intf_init(). It was removed as a cleanup,
> now we can reintroduce it.
>
Thanks, that will address all these concerns.
I wanted to get agreement before re-introducing it and also make sure
there was no other way.
>>
>> Ideally even I would like to assign this op only for core_rev >=7 but
>> that information is no longer available. We would have to start
>> passing the major and minor versions to _setup_intf_ops() to go with
>> that approach. So without making all of those changes, the only way I
>> had was to assign the op unconditionally but call it only for
>> major_rev >= 7.
>>
>> Passing core_rev to the op itself so that we can write the register
>> only for core_rev >=7 is an option but then what if some bits start
>> becoming usable only after minor rev. then we will have to start
>> passing major and minor rev to program_datapath too. Again getting
>> little messy.
>>
>> I am open to ideas to achieve the goal of assigning this op only for
>> core_rev >=7 other than what I wrote above.
>>
>>>
>>>> + struct intf_dpu_datapath_cfg datapath_cfg = { 0 };
>>>
>>> No need for `0' in the init, empty braces would be enough.
>>>
>>
>> ack.
>>
>>>> +
>>>> + datapath_cfg.data_compress = true;
>>>> + phys_enc->hw_intf->ops.program_datapath(phys_enc->hw_intf,
>>>> &datapath_cfg);
>>>> + }
>>>> }
>>>> static void dpu_encoder_phys_cmd_pp_tx_done_irq(void *arg, int
>>>> irq_idx)
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>>> index 5b0f6627e29b..85333df08fbc 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>>> @@ -513,11 +513,13 @@ static void
>>>> dpu_hw_intf_disable_autorefresh(struct dpu_hw_intf *intf,
>>>> }
>>>> -static void dpu_hw_intf_enable_compression(struct dpu_hw_intf *ctx)
>>>> +static void dpu_hw_intf_program_datapath(struct dpu_hw_intf *ctx,
>>>> + struct intf_dpu_datapath_cfg *datapath_cfg)
>>>> {
>>>> u32 intf_cfg2 = DPU_REG_READ(&ctx->hw, INTF_CONFIG2);
>>>> - intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
>>>> + if (datapath_cfg->data_compress)
>>>> + intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
>>>> DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, intf_cfg2);
>>>> }
>>>> @@ -543,8 +545,7 @@ static void _setup_intf_ops(struct
>>>> dpu_hw_intf_ops *ops,
>>>> ops->disable_autorefresh = dpu_hw_intf_disable_autorefresh;
>>>> }
>>>> - if (cap & BIT(DPU_INTF_DATA_COMPRESS))
>>>> - ops->enable_compression = dpu_hw_intf_enable_compression;
>>>> + ops->program_datapath = dpu_hw_intf_program_datapath;
>>>
>>> The `core_major_version >= 7' should either be here or in the
>>> callback itself.
>>>
>>
>> Yes, ideally I would like it like that but please see above why I
>> couldnt do it.
>>
>>>> }
>>>> struct dpu_hw_intf *dpu_hw_intf_init(const struct dpu_intf_cfg *cfg,
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>>>> index 99e21c4137f9..f736dca38463 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>>>> @@ -48,6 +48,11 @@ struct intf_status {
>>>> u32 line_count; /* current line count including
>>>> blanking */
>>>> };
>>>> +struct intf_dpu_datapath_cfg {
>>>> + u8 data_compress; /* enable data compress between dpu and
>>>> dsi */
>>>> + /* can be expanded for other programmable bits */
>>>> +};
>>>
>>> I'd say, dpu_datapath is too generic. What about intf_cmd_mode_cfg?
>>>
>>
>> The goal was to keep it generic. Its actually the handshake between
>> DPU and interface datapath so I chose that name.
>
> Do you have plans of using it for the video mode?
>
No because we didnt want to touch the video mode path as that was
discussed in the widebus series already.
Ok, I am fine with intf_cmd_mode_cfg in that case.
>>
>> This is not specific to command mode and intf_cfg is already there so
>> I chose that one :)
>>
>>>> +
>>>> /**
>>>> * struct dpu_hw_intf_ops : Interface to the interface Hw driver
>>>> functions
>>>> * Assumption is these functions will be called after clocks are
>>>> enabled
>>>> @@ -70,7 +75,7 @@ struct intf_status {
>>>> * @get_autorefresh: Retrieve autorefresh config from
>>>> hardware
>>>> * Return: 0 on success, -ETIMEDOUT
>>>> on timeout
>>>> * @vsync_sel: Select vsync signal for
>>>> tear-effect configuration
>>>> - * @enable_compression: Enable data compression
>>>> + * @program_datapath: Program the DPU to interface
>>>> datapath for relevant chipsets
>>>> */
>>>> struct dpu_hw_intf_ops {
>>>> void (*setup_timing_gen)(struct dpu_hw_intf *intf,
>>>> @@ -108,7 +113,14 @@ struct dpu_hw_intf_ops {
>>>> */
>>>> void (*disable_autorefresh)(struct dpu_hw_intf *intf, uint32_t
>>>> encoder_id, u16 vdisplay);
>>>> - void (*enable_compression)(struct dpu_hw_intf *intf);
>>>> + /**
>>>> + * Program the DPU to intf datapath by specifying
>>>> + * which of the settings need to be programmed for
>>>> + * use-cases which need DPU-intf handshake such as
>>>> + * widebus, compression etc.
>>>
>>> This is not a valid kerneldoc.
>>>
>>
>> hmmm ... ok so just // ?
>>
>> I referred disable_autorefresh from above and did the same.
>>
>>>> + */
>>>> + void (*program_datapath)(struct dpu_hw_intf *intf,
>>>> + struct intf_dpu_datapath_cfg *datapath_cfg);
>>>> };
>>>> struct dpu_hw_intf {
>>>
>
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2023-06-30 3:30 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-29 19:29 [PATCH v3 1/3] drm/msm/dpu: re-introduce dpu core revision to the catalog Abhinav Kumar
2023-06-29 19:29 ` Abhinav Kumar
2023-06-29 19:29 ` [PATCH v3 2/3] drm/msm/dpu: use dpu core's major version to enable data compress Abhinav Kumar
2023-06-29 19:29 ` Abhinav Kumar
2023-06-30 0:20 ` Dmitry Baryshkov
2023-06-30 0:20 ` Dmitry Baryshkov
2023-06-30 3:07 ` Abhinav Kumar
2023-06-30 3:07 ` Abhinav Kumar
2023-06-30 3:22 ` Dmitry Baryshkov
2023-06-30 3:22 ` Dmitry Baryshkov
2023-06-30 3:30 ` Abhinav Kumar
2023-06-30 3:30 ` Abhinav Kumar
2023-06-29 19:29 ` [PATCH v3 3/3] drm/msm/dpu: drop DPU_INTF_DATA_COMPRESS from dpu catalog Abhinav Kumar
2023-06-29 19:29 ` Abhinav Kumar
2023-06-30 0:20 ` Dmitry Baryshkov
2023-06-30 0:20 ` Dmitry Baryshkov
2023-06-30 0:13 ` [PATCH v3 1/3] drm/msm/dpu: re-introduce dpu core revision to the catalog Dmitry Baryshkov
2023-06-30 0:13 ` Dmitry Baryshkov
2023-06-30 3:15 ` Abhinav Kumar
2023-06-30 3:15 ` Abhinav Kumar
2023-06-30 0:24 ` Dmitry Baryshkov
2023-06-30 0:24 ` Dmitry Baryshkov
2023-06-30 3:17 ` Abhinav Kumar
2023-06-30 3:17 ` Abhinav Kumar
2023-06-30 3:19 ` Dmitry Baryshkov
2023-06-30 3:19 ` Dmitry Baryshkov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.