[PATCH 0/6] drm/msm/dpu: use UBWC data from MDSS driver

[PATCH 0/6] drm/msm/dpu: use UBWC data from MDSS driver

[Dmitry Baryshkov]

Both DPU and MDSS programming requires knowledge of some of UBWC
parameters. This results in duplication of UBWC data between MDSS and
DPU drivers. To remove such duplication and make the driver more
error-prone, export respective configuration from the MDSS driver and
make DPU use it, instead of bundling a copy of such data.

Dmitry Baryshkov (6):
  drm/msm/mdss: correct UBWC programming for SM8550
  drm/msm/mdss: rename ubwc_version to ubwc_enc_version
  drm/msm/mdss: export UBWC data
  drm/msm/mdss: populate missing data
  drm/msm/dpu: use MDSS data for programming SSPP
  drm/msm/dpu: drop UBWC configuration

 .../msm/disp/dpu1/catalog/dpu_3_0_msm8998.h   |  6 --
 .../msm/disp/dpu1/catalog/dpu_4_0_sdm845.h    |  6 --
 .../msm/disp/dpu1/catalog/dpu_5_0_sm8150.h    |  6 --
 .../msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h   |  6 --
 .../msm/disp/dpu1/catalog/dpu_6_0_sm8250.h    |  7 --
 .../msm/disp/dpu1/catalog/dpu_6_2_sc7180.h    |  6 --
 .../msm/disp/dpu1/catalog/dpu_6_3_sm6115.h    |  7 --
 .../msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h   |  5 --
 .../msm/disp/dpu1/catalog/dpu_7_0_sm8350.h    |  6 --
 .../msm/disp/dpu1/catalog/dpu_7_2_sc7280.h    |  7 --
 .../msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h  |  7 --
 .../msm/disp/dpu1/catalog/dpu_8_1_sm8450.h    |  7 --
 .../msm/disp/dpu1/catalog/dpu_9_0_sm8550.h    |  6 --
 .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h    | 15 ----
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c   | 18 ++--
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h   |  7 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c       | 16 +++-
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h       |  1 +
 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c        |  3 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h        |  2 +
 drivers/gpu/drm/msm/msm_mdss.c                | 90 ++++++++++++-------
 drivers/gpu/drm/msm/msm_mdss.h                | 27 ++++++
 22 files changed, 122 insertions(+), 139 deletions(-)
 create mode 100644 drivers/gpu/drm/msm/msm_mdss.h

-- 
2.39.2

[PATCH 1/6] drm/msm/mdss: correct UBWC programming for SM8550

[Dmitry Baryshkov]

The SM8550 platform employs newer UBWC decoder, which requires slightly
different programming.

Fixes: a2f33995c19d ("drm/msm: mdss: add support for SM8550")
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/msm_mdss.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
index e8c93731aaa1..4ae6fac20e48 100644
--- a/drivers/gpu/drm/msm/msm_mdss.c
+++ b/drivers/gpu/drm/msm/msm_mdss.c
@@ -189,6 +189,7 @@ static int _msm_mdss_irq_domain_add(struct msm_mdss *msm_mdss)
 #define UBWC_2_0 0x20000000
 #define UBWC_3_0 0x30000000
 #define UBWC_4_0 0x40000000
+#define UBWC_4_3 0x40030000
 
 static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss)
 {
@@ -227,7 +228,10 @@ static void msm_mdss_setup_ubwc_dec_40(struct msm_mdss *msm_mdss)
 		writel_relaxed(1, msm_mdss->mmio + UBWC_CTRL_2);
 		writel_relaxed(0, msm_mdss->mmio + UBWC_PREDICTION_MODE);
 	} else {
-		writel_relaxed(2, msm_mdss->mmio + UBWC_CTRL_2);
+		if (data->ubwc_dec_version == UBWC_4_3)
+			writel_relaxed(3, msm_mdss->mmio + UBWC_CTRL_2);
+		else
+			writel_relaxed(2, msm_mdss->mmio + UBWC_CTRL_2);
 		writel_relaxed(1, msm_mdss->mmio + UBWC_PREDICTION_MODE);
 	}
 }
@@ -271,6 +275,7 @@ static int msm_mdss_enable(struct msm_mdss *msm_mdss)
 		msm_mdss_setup_ubwc_dec_30(msm_mdss);
 		break;
 	case UBWC_4_0:
+	case UBWC_4_3:
 		msm_mdss_setup_ubwc_dec_40(msm_mdss);
 		break;
 	default:
@@ -561,6 +566,16 @@ static const struct msm_mdss_data sm8250_data = {
 	.macrotile_mode = 1,
 };
 
+static const struct msm_mdss_data sm8550_data = {
+	.ubwc_version = UBWC_4_0,
+	.ubwc_dec_version = UBWC_4_3,
+	.ubwc_swizzle = 6,
+	.ubwc_static = 1,
+	/* TODO: highest_bank_bit = 2 for LP_DDR4 */
+	.highest_bank_bit = 3,
+	.macrotile_mode = 1,
+};
+
 static const struct of_device_id mdss_dt_match[] = {
 	{ .compatible = "qcom,mdss" },
 	{ .compatible = "qcom,msm8998-mdss" },
@@ -575,7 +590,7 @@ static const struct of_device_id mdss_dt_match[] = {
 	{ .compatible = "qcom,sm8250-mdss", .data = &sm8250_data },
 	{ .compatible = "qcom,sm8350-mdss", .data = &sm8250_data },
 	{ .compatible = "qcom,sm8450-mdss", .data = &sm8250_data },
-	{ .compatible = "qcom,sm8550-mdss", .data = &sm8250_data },
+	{ .compatible = "qcom,sm8550-mdss", .data = &sm8550_data },
 	{}
 };
 MODULE_DEVICE_TABLE(of, mdss_dt_match);
-- 
2.39.2

[PATCH 2/6] drm/msm/mdss: rename ubwc_version to ubwc_enc_version

[Dmitry Baryshkov]

Rename the ubwc_version field to ubwc_enc_version, it denotes the
version of the UBWC encoder, not the "UBWC version".

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/msm_mdss.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
index 4ae6fac20e48..d1e57099b517 100644
--- a/drivers/gpu/drm/msm/msm_mdss.c
+++ b/drivers/gpu/drm/msm/msm_mdss.c
@@ -27,7 +27,7 @@
 #define MIN_IB_BW	400000000UL /* Min ib vote 400MB */
 
 struct msm_mdss_data {
-	u32 ubwc_version;
+	u32 ubwc_enc_version;
 	/* can be read from register 0x58 */
 	u32 ubwc_dec_version;
 	u32 ubwc_swizzle;
@@ -205,10 +205,10 @@ static void msm_mdss_setup_ubwc_dec_30(struct msm_mdss *msm_mdss)
 		    (data->highest_bank_bit & 0x3) << 4 |
 		    (data->macrotile_mode & 0x1) << 12;
 
-	if (data->ubwc_version == UBWC_3_0)
+	if (data->ubwc_enc_version == UBWC_3_0)
 		value |= BIT(10);
 
-	if (data->ubwc_version == UBWC_1_0)
+	if (data->ubwc_enc_version == UBWC_1_0)
 		value |= BIT(8);
 
 	writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
@@ -224,7 +224,7 @@ static void msm_mdss_setup_ubwc_dec_40(struct msm_mdss *msm_mdss)
 
 	writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
 
-	if (data->ubwc_version == UBWC_3_0) {
+	if (data->ubwc_enc_version == UBWC_3_0) {
 		writel_relaxed(1, msm_mdss->mmio + UBWC_CTRL_2);
 		writel_relaxed(0, msm_mdss->mmio + UBWC_PREDICTION_MODE);
 	} else {
@@ -507,13 +507,13 @@ static int mdss_remove(struct platform_device *pdev)
 }
 
 static const struct msm_mdss_data sc7180_data = {
-	.ubwc_version = UBWC_2_0,
+	.ubwc_enc_version = UBWC_2_0,
 	.ubwc_dec_version = UBWC_2_0,
 	.ubwc_static = 0x1e,
 };
 
 static const struct msm_mdss_data sc7280_data = {
-	.ubwc_version = UBWC_3_0,
+	.ubwc_enc_version = UBWC_3_0,
 	.ubwc_dec_version = UBWC_4_0,
 	.ubwc_swizzle = 6,
 	.ubwc_static = 1,
@@ -522,14 +522,14 @@ static const struct msm_mdss_data sc7280_data = {
 };
 
 static const struct msm_mdss_data sc8180x_data = {
-	.ubwc_version = UBWC_3_0,
+	.ubwc_enc_version = UBWC_3_0,
 	.ubwc_dec_version = UBWC_3_0,
 	.highest_bank_bit = 3,
 	.macrotile_mode = 1,
 };
 
 static const struct msm_mdss_data sc8280xp_data = {
-	.ubwc_version = UBWC_4_0,
+	.ubwc_enc_version = UBWC_4_0,
 	.ubwc_dec_version = UBWC_4_0,
 	.ubwc_swizzle = 6,
 	.ubwc_static = 1,
@@ -538,26 +538,26 @@ static const struct msm_mdss_data sc8280xp_data = {
 };
 
 static const struct msm_mdss_data sdm845_data = {
-	.ubwc_version = UBWC_2_0,
+	.ubwc_enc_version = UBWC_2_0,
 	.ubwc_dec_version = UBWC_2_0,
 	.highest_bank_bit = 2,
 };
 
 static const struct msm_mdss_data sm8150_data = {
-	.ubwc_version = UBWC_3_0,
+	.ubwc_enc_version = UBWC_3_0,
 	.ubwc_dec_version = UBWC_3_0,
 	.highest_bank_bit = 2,
 };
 
 static const struct msm_mdss_data sm6115_data = {
-	.ubwc_version = UBWC_1_0,
+	.ubwc_enc_version = UBWC_1_0,
 	.ubwc_dec_version = UBWC_2_0,
 	.ubwc_swizzle = 7,
 	.ubwc_static = 0x11f,
 };
 
 static const struct msm_mdss_data sm8250_data = {
-	.ubwc_version = UBWC_4_0,
+	.ubwc_enc_version = UBWC_4_0,
 	.ubwc_dec_version = UBWC_4_0,
 	.ubwc_swizzle = 6,
 	.ubwc_static = 1,
@@ -567,7 +567,7 @@ static const struct msm_mdss_data sm8250_data = {
 };
 
 static const struct msm_mdss_data sm8550_data = {
-	.ubwc_version = UBWC_4_0,
+	.ubwc_enc_version = UBWC_4_0,
 	.ubwc_dec_version = UBWC_4_3,
 	.ubwc_swizzle = 6,
 	.ubwc_static = 1,
-- 
2.39.2

[PATCH 3/6] drm/msm/mdss: export UBWC data

[Dmitry Baryshkov]

DPU programming requires knowledge of some of UBWC parameters. This
results in duplication of UBWC data between MDSS and DPU drivers. Export
the required data from MDSS driver.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/msm_mdss.c | 30 +++++++++++++-----------------
 drivers/gpu/drm/msm/msm_mdss.h | 27 +++++++++++++++++++++++++++
 2 files changed, 40 insertions(+), 17 deletions(-)
 create mode 100644 drivers/gpu/drm/msm/msm_mdss.h

diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
index d1e57099b517..ed836c659688 100644
--- a/drivers/gpu/drm/msm/msm_mdss.c
+++ b/drivers/gpu/drm/msm/msm_mdss.c
@@ -13,7 +13,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/reset.h>
 
-#include "msm_drv.h"
+#include "msm_mdss.h"
 #include "msm_kms.h"
 
 #define HW_REV				0x0
@@ -26,16 +26,6 @@
 
 #define MIN_IB_BW	400000000UL /* Min ib vote 400MB */
 
-struct msm_mdss_data {
-	u32 ubwc_enc_version;
-	/* can be read from register 0x58 */
-	u32 ubwc_dec_version;
-	u32 ubwc_swizzle;
-	u32 ubwc_static;
-	u32 highest_bank_bit;
-	u32 macrotile_mode;
-};
-
 struct msm_mdss {
 	struct device *dev;
 
@@ -185,12 +175,6 @@ static int _msm_mdss_irq_domain_add(struct msm_mdss *msm_mdss)
 	return 0;
 }
 
-#define UBWC_1_0 0x10000000
-#define UBWC_2_0 0x20000000
-#define UBWC_3_0 0x30000000
-#define UBWC_4_0 0x40000000
-#define UBWC_4_3 0x40030000
-
 static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss)
 {
 	const struct msm_mdss_data *data = msm_mdss->mdss_data;
@@ -236,6 +220,18 @@ static void msm_mdss_setup_ubwc_dec_40(struct msm_mdss *msm_mdss)
 	}
 }
 
+const struct msm_mdss_data *msm_mdss_get_mdss_data(struct device *dev)
+{
+	struct msm_mdss *mdss;
+
+	if (!dev)
+		return ERR_PTR(-EINVAL);
+
+	mdss = dev_get_drvdata(dev);
+
+	return mdss->mdss_data;
+}
+
 static int msm_mdss_enable(struct msm_mdss *msm_mdss)
 {
 	int ret;
diff --git a/drivers/gpu/drm/msm/msm_mdss.h b/drivers/gpu/drm/msm/msm_mdss.h
new file mode 100644
index 000000000000..02bbab42adbc
--- /dev/null
+++ b/drivers/gpu/drm/msm/msm_mdss.h
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2018, The Linux Foundation
+ */
+
+#ifndef __MSM_MDSS_H__
+#define __MSM_MDSS_H__
+
+struct msm_mdss_data {
+	u32 ubwc_enc_version;
+	/* can be read from register 0x58 */
+	u32 ubwc_dec_version;
+	u32 ubwc_swizzle;
+	u32 ubwc_static;
+	u32 highest_bank_bit;
+	u32 macrotile_mode;
+};
+
+#define UBWC_1_0 0x10000000
+#define UBWC_2_0 0x20000000
+#define UBWC_3_0 0x30000000
+#define UBWC_4_0 0x40000000
+#define UBWC_4_3 0x40030000
+
+const struct msm_mdss_data *msm_mdss_get_mdss_data(struct device *dev);
+
+#endif /* __MSM_MDSS_H__ */
-- 
2.39.2

[PATCH 4/6] drm/msm/mdss: populate missing data

[Dmitry Baryshkov]

As we are going to use MDSS data for DPU programming, populate missing
MDSS data. The UBWC 1.0 and no UBWC cases do not require MDSS
programming, so skip them.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/msm_mdss.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
index ed836c659688..9bb7be4b9ebb 100644
--- a/drivers/gpu/drm/msm/msm_mdss.c
+++ b/drivers/gpu/drm/msm/msm_mdss.c
@@ -264,6 +264,10 @@ static int msm_mdss_enable(struct msm_mdss *msm_mdss)
 	 * UBWC_n and the rest of params comes from hw data.
 	 */
 	switch (msm_mdss->mdss_data->ubwc_dec_version) {
+	case 0: /* no UBWC */
+	case UBWC_1_0:
+		/* do nothing */
+		break;
 	case UBWC_2_0:
 		msm_mdss_setup_ubwc_dec_20(msm_mdss);
 		break;
@@ -502,10 +506,22 @@ static int mdss_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct msm_mdss_data msm8998_data = {
+	.ubwc_enc_version = UBWC_1_0,
+	.ubwc_dec_version = UBWC_1_0,
+	.highest_bank_bit = 1,
+};
+
+static const struct msm_mdss_data qcm2290_data = {
+	/* no UBWC */
+	.highest_bank_bit = 0x2,
+};
+
 static const struct msm_mdss_data sc7180_data = {
 	.ubwc_enc_version = UBWC_2_0,
 	.ubwc_dec_version = UBWC_2_0,
 	.ubwc_static = 0x1e,
+	.highest_bank_bit = 0x3,
 };
 
 static const struct msm_mdss_data sc7280_data = {
@@ -550,6 +566,7 @@ static const struct msm_mdss_data sm6115_data = {
 	.ubwc_dec_version = UBWC_2_0,
 	.ubwc_swizzle = 7,
 	.ubwc_static = 0x11f,
+	.highest_bank_bit = 0x1,
 };
 
 static const struct msm_mdss_data sm8250_data = {
@@ -574,8 +591,8 @@ static const struct msm_mdss_data sm8550_data = {
 
 static const struct of_device_id mdss_dt_match[] = {
 	{ .compatible = "qcom,mdss" },
-	{ .compatible = "qcom,msm8998-mdss" },
-	{ .compatible = "qcom,qcm2290-mdss" },
+	{ .compatible = "qcom,msm8998-mdss", .data = &msm8998_data },
+	{ .compatible = "qcom,qcm2290-mdss", .data = &qcm2290_data },
 	{ .compatible = "qcom,sdm845-mdss", .data = &sdm845_data },
 	{ .compatible = "qcom,sc7180-mdss", .data = &sc7180_data },
 	{ .compatible = "qcom,sc7280-mdss", .data = &sc7280_data },
-- 
2.39.2

[PATCH 5/6] drm/msm/dpu: use MDSS data for programming SSPP

[Dmitry Baryshkov]

Switch to using data from MDSS driver to program the SSPP fetch and UBWC
configuration.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 18 +++++++++++-------
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h |  7 +++++--
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     | 16 +++++++++++++++-
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h     |  1 +
 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c      |  3 ++-
 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h      |  2 ++
 6 files changed, 36 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
index cf70a9bd1034..bfd82c2921af 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
@@ -8,6 +8,8 @@
 #include "dpu_hw_sspp.h"
 #include "dpu_kms.h"
 
+#include "msm_mdss.h"
+
 #include <drm/drm_file.h>
 
 #define DPU_FETCH_CONFIG_RESET_VALUE   0x00000087
@@ -308,26 +310,26 @@ static void dpu_hw_sspp_setup_format(struct dpu_sw_pipe *pipe,
 		DPU_REG_WRITE(c, SSPP_FETCH_CONFIG,
 			DPU_FETCH_CONFIG_RESET_VALUE |
 			ctx->ubwc->highest_bank_bit << 18);
-		switch (ctx->ubwc->ubwc_version) {
-		case DPU_HW_UBWC_VER_10:
+		switch (ctx->ubwc->ubwc_enc_version) {
+		case UBWC_1_0:
 			fast_clear = fmt->alpha_enable ? BIT(31) : 0;
 			DPU_REG_WRITE(c, SSPP_UBWC_STATIC_CTRL,
 					fast_clear | (ctx->ubwc->ubwc_swizzle & 0x1) |
 					BIT(8) |
 					(ctx->ubwc->highest_bank_bit << 4));
 			break;
-		case DPU_HW_UBWC_VER_20:
+		case UBWC_2_0:
 			fast_clear = fmt->alpha_enable ? BIT(31) : 0;
 			DPU_REG_WRITE(c, SSPP_UBWC_STATIC_CTRL,
 					fast_clear | (ctx->ubwc->ubwc_swizzle) |
 					(ctx->ubwc->highest_bank_bit << 4));
 			break;
-		case DPU_HW_UBWC_VER_30:
+		case UBWC_3_0:
 			DPU_REG_WRITE(c, SSPP_UBWC_STATIC_CTRL,
 					BIT(30) | (ctx->ubwc->ubwc_swizzle) |
 					(ctx->ubwc->highest_bank_bit << 4));
 			break;
-		case DPU_HW_UBWC_VER_40:
+		case UBWC_4_0:
 			DPU_REG_WRITE(c, SSPP_UBWC_STATIC_CTRL,
 					DPU_FORMAT_IS_YUV(fmt) ? 0 : BIT(30));
 			break;
@@ -793,7 +795,9 @@ static const struct dpu_sspp_cfg *_sspp_offset(enum dpu_sspp sspp,
 }
 
 struct dpu_hw_sspp *dpu_hw_sspp_init(enum dpu_sspp idx,
-		void __iomem *addr, const struct dpu_mdss_cfg *catalog)
+				     void __iomem *addr,
+				     const struct dpu_mdss_cfg *catalog,
+				     const struct msm_mdss_data *mdss_data)
 {
 	struct dpu_hw_sspp *hw_pipe;
 	const struct dpu_sspp_cfg *cfg;
@@ -813,7 +817,7 @@ struct dpu_hw_sspp *dpu_hw_sspp_init(enum dpu_sspp idx,
 
 	/* Assign ops */
 	hw_pipe->catalog = catalog;
-	hw_pipe->ubwc = catalog->ubwc;
+	hw_pipe->ubwc = mdss_data;
 	hw_pipe->idx = idx;
 	hw_pipe->cap = cfg;
 	_setup_layer_ops(hw_pipe, hw_pipe->cap->features);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
index 74b98b6b3bc3..8d4ae9d24674 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
@@ -351,7 +351,7 @@ struct dpu_hw_sspp {
 	struct dpu_hw_blk base;
 	struct dpu_hw_blk_reg_map hw;
 	const struct dpu_mdss_cfg *catalog;
-	const struct dpu_ubwc_cfg *ubwc;
+	const struct msm_mdss_data *ubwc;
 
 	/* Pipe */
 	enum dpu_sspp idx;
@@ -368,9 +368,12 @@ struct dpu_kms;
  * @idx:  Pipe index for which driver object is required
  * @addr: Mapped register io address of MDP
  * @catalog : Pointer to mdss catalog data
+ * @mdss_data: UBWC / MDSS configuration data
  */
 struct dpu_hw_sspp *dpu_hw_sspp_init(enum dpu_sspp idx,
-		void __iomem *addr, const struct dpu_mdss_cfg *catalog);
+				     void __iomem *addr,
+				     const struct dpu_mdss_cfg *catalog,
+				     const struct msm_mdss_data *mdss_data);
 
 /**
  * dpu_hw_sspp_destroy(): Destroys SSPP driver context
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 0e7a68714e9e..a4293dc0b61b 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -22,6 +22,7 @@
 
 #include "msm_drv.h"
 #include "msm_mmu.h"
+#include "msm_mdss.h"
 #include "msm_gem.h"
 #include "disp/msm_disp_snapshot.h"
 
@@ -1066,7 +1067,20 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
 		goto power_error;
 	}
 
-	rc = dpu_rm_init(&dpu_kms->rm, dpu_kms->catalog, dpu_kms->mmio);
+	dpu_kms->mdss = msm_mdss_get_mdss_data(dpu_kms->pdev->dev.parent);
+	if (IS_ERR(dpu_kms->mdss)) {
+		rc = PTR_ERR(dpu_kms->mdss);
+		DPU_ERROR("failed to get MDSS data: %d\n", rc);
+		goto power_error;
+	}
+
+	if (!dpu_kms->mdss) {
+		rc = -EINVAL;
+		DPU_ERROR("NULL MDSS data\n");
+		goto power_error;
+	}
+
+	rc = dpu_rm_init(&dpu_kms->rm, dpu_kms->catalog, dpu_kms->mdss, dpu_kms->mmio);
 	if (rc) {
 		DPU_ERROR("rm init failed: %d\n", rc);
 		goto power_error;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
index aca39a4689f4..797b4ff3e806 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
@@ -69,6 +69,7 @@ struct dpu_kms {
 	struct msm_kms base;
 	struct drm_device *dev;
 	const struct dpu_mdss_cfg *catalog;
+	const struct msm_mdss_data *mdss;
 
 	/* io/register spaces: */
 	void __iomem *mmio, *vbif[VBIF_MAX], *reg_dma;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
index f4dda88a73f7..9ee493465c4b 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
@@ -100,6 +100,7 @@ int dpu_rm_destroy(struct dpu_rm *rm)
 
 int dpu_rm_init(struct dpu_rm *rm,
 		const struct dpu_mdss_cfg *cat,
+		const struct msm_mdss_data *mdss_data,
 		void __iomem *mmio)
 {
 	int rc, i;
@@ -268,7 +269,7 @@ int dpu_rm_init(struct dpu_rm *rm,
 			continue;
 		}
 
-		hw = dpu_hw_sspp_init(sspp->id, mmio, cat);
+		hw = dpu_hw_sspp_init(sspp->id, mmio, cat, mdss_data);
 		if (IS_ERR(hw)) {
 			rc = PTR_ERR(hw);
 			DPU_ERROR("failed sspp object creation: err %d\n", rc);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
index d62c2edb2460..2b551566cbf4 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
@@ -40,11 +40,13 @@ struct dpu_rm {
  *	for all HW blocks.
  * @rm: DPU Resource Manager handle
  * @cat: Pointer to hardware catalog
+ * @mdss_data: Pointer to MDSS / UBWC configuration
  * @mmio: mapped register io address of MDP
  * @Return: 0 on Success otherwise -ERROR
  */
 int dpu_rm_init(struct dpu_rm *rm,
 		const struct dpu_mdss_cfg *cat,
+		const struct msm_mdss_data *mdss_data,
 		void __iomem *mmio);
 
 /**
-- 
2.39.2

[PATCH 6/6] drm/msm/dpu: drop UBWC configuration

[Dmitry Baryshkov]

As the DPU driver has switched to fetching data from MDSS driver, we can
now drop the UBWC and highest_bank_bit parts of the DPU hw catalog.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 .../drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h   |  6 ------
 .../drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h    |  6 ------
 .../drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h    |  6 ------
 .../drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h   |  6 ------
 .../drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h    |  7 -------
 .../drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h    |  6 ------
 .../drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h    |  7 -------
 .../drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h   |  5 -----
 .../drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h    |  6 ------
 .../drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h    |  7 -------
 .../drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h  |  7 -------
 .../drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h    |  7 -------
 .../drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h    |  6 ------
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h    | 15 ---------------
 14 files changed, 97 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 bdcd554fc8a8..59fa5a376831 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
@@ -21,11 +21,6 @@ static const struct dpu_caps msm8998_dpu_caps = {
 	.max_vdeci_exp = MAX_VERT_DECIMATION,
 };
 
-static const struct dpu_ubwc_cfg msm8998_ubwc_cfg = {
-	.ubwc_version = DPU_HW_UBWC_VER_10,
-	.highest_bank_bit = 0x2,
-};
-
 static const struct dpu_mdp_cfg msm8998_mdp[] = {
 	{
 	.name = "top_0", .id = MDP_TOP,
@@ -178,7 +173,6 @@ static const struct dpu_perf_cfg msm8998_perf_data = {
 
 const struct dpu_mdss_cfg dpu_msm8998_cfg = {
 	.caps = &msm8998_dpu_caps,
-	.ubwc = &msm8998_ubwc_cfg,
 	.mdp_count = ARRAY_SIZE(msm8998_mdp),
 	.mdp = msm8998_mdp,
 	.ctl_count = ARRAY_SIZE(msm8998_ctl),
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 ceca741e93c9..f34ef20aafe0 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
@@ -21,11 +21,6 @@ static const struct dpu_caps sdm845_dpu_caps = {
 	.max_vdeci_exp = MAX_VERT_DECIMATION,
 };
 
-static const struct dpu_ubwc_cfg sdm845_ubwc_cfg = {
-	.ubwc_version = DPU_HW_UBWC_VER_20,
-	.highest_bank_bit = 0x2,
-};
-
 static const struct dpu_mdp_cfg sdm845_mdp[] = {
 	{
 	.name = "top_0", .id = MDP_TOP,
@@ -176,7 +171,6 @@ static const struct dpu_perf_cfg sdm845_perf_data = {
 
 const struct dpu_mdss_cfg dpu_sdm845_cfg = {
 	.caps = &sdm845_dpu_caps,
-	.ubwc = &sdm845_ubwc_cfg,
 	.mdp_count = ARRAY_SIZE(sdm845_mdp),
 	.mdp = sdm845_mdp,
 	.ctl_count = ARRAY_SIZE(sdm845_ctl),
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 42b0e58624d0..a51209603243 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
@@ -21,11 +21,6 @@ static const struct dpu_caps sm8150_dpu_caps = {
 	.max_vdeci_exp = MAX_VERT_DECIMATION,
 };
 
-static const struct dpu_ubwc_cfg sm8150_ubwc_cfg = {
-	.ubwc_version = DPU_HW_UBWC_VER_30,
-	.highest_bank_bit = 0x2,
-};
-
 static const struct dpu_mdp_cfg sm8150_mdp[] = {
 	{
 	.name = "top_0", .id = MDP_TOP,
@@ -199,7 +194,6 @@ static const struct dpu_perf_cfg sm8150_perf_data = {
 
 const struct dpu_mdss_cfg dpu_sm8150_cfg = {
 	.caps = &sm8150_dpu_caps,
-	.ubwc = &sm8150_ubwc_cfg,
 	.mdp_count = ARRAY_SIZE(sm8150_mdp),
 	.mdp = sm8150_mdp,
 	.ctl_count = ARRAY_SIZE(sm8150_ctl),
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 e3bdfe7b30f1..574e1e45941d 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
@@ -21,11 +21,6 @@ static const struct dpu_caps sc8180x_dpu_caps = {
 	.max_vdeci_exp = MAX_VERT_DECIMATION,
 };
 
-static const struct dpu_ubwc_cfg sc8180x_ubwc_cfg = {
-	.ubwc_version = DPU_HW_UBWC_VER_30,
-	.highest_bank_bit = 0x3,
-};
-
 static const struct dpu_mdp_cfg sc8180x_mdp[] = {
 	{
 	.name = "top_0", .id = MDP_TOP,
@@ -181,7 +176,6 @@ static const struct dpu_perf_cfg sc8180x_perf_data = {
 
 const struct dpu_mdss_cfg dpu_sc8180x_cfg = {
 	.caps = &sc8180x_dpu_caps,
-	.ubwc = &sc8180x_ubwc_cfg,
 	.mdp_count = ARRAY_SIZE(sc8180x_mdp),
 	.mdp = sc8180x_mdp,
 	.ctl_count = ARRAY_SIZE(sc8180x_ctl),
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 ed130582873c..2df9fd4080bb 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
@@ -19,12 +19,6 @@ static const struct dpu_caps sm8250_dpu_caps = {
 	.pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
 };
 
-static const struct dpu_ubwc_cfg sm8250_ubwc_cfg = {
-	.ubwc_version = DPU_HW_UBWC_VER_40,
-	.highest_bank_bit = 0x3, /* TODO: 2 for LP_DDR4 */
-	.ubwc_swizzle = 0x6,
-};
-
 static const struct dpu_mdp_cfg sm8250_mdp[] = {
 	{
 	.name = "top_0", .id = MDP_TOP,
@@ -205,7 +199,6 @@ static const struct dpu_perf_cfg sm8250_perf_data = {
 
 const struct dpu_mdss_cfg dpu_sm8250_cfg = {
 	.caps = &sm8250_dpu_caps,
-	.ubwc = &sm8250_ubwc_cfg,
 	.mdp_count = ARRAY_SIZE(sm8250_mdp),
 	.mdp = sm8250_mdp,
 	.ctl_count = ARRAY_SIZE(sm8250_ctl),
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 a46b11730a4d..8a044c411a4d 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
@@ -17,11 +17,6 @@ static const struct dpu_caps sc7180_dpu_caps = {
 	.pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
 };
 
-static const struct dpu_ubwc_cfg sc7180_ubwc_cfg = {
-	.ubwc_version = DPU_HW_UBWC_VER_20,
-	.highest_bank_bit = 0x3,
-};
-
 static const struct dpu_mdp_cfg sc7180_mdp[] = {
 	{
 	.name = "top_0", .id = MDP_TOP,
@@ -124,7 +119,6 @@ static const struct dpu_perf_cfg sc7180_perf_data = {
 
 const struct dpu_mdss_cfg dpu_sc7180_cfg = {
 	.caps = &sc7180_dpu_caps,
-	.ubwc = &sc7180_ubwc_cfg,
 	.mdp_count = ARRAY_SIZE(sc7180_mdp),
 	.mdp = sc7180_mdp,
 	.ctl_count = ARRAY_SIZE(sc7180_ctl),
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 988d820f7ef2..e92ab625a343 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
@@ -17,12 +17,6 @@ static const struct dpu_caps sm6115_dpu_caps = {
 	.pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
 };
 
-static const struct dpu_ubwc_cfg sm6115_ubwc_cfg = {
-	.ubwc_version = DPU_HW_UBWC_VER_10,
-	.highest_bank_bit = 0x1,
-	.ubwc_swizzle = 0x7,
-};
-
 static const struct dpu_mdp_cfg sm6115_mdp[] = {
 	{
 	.name = "top_0", .id = MDP_TOP,
@@ -101,7 +95,6 @@ static const struct dpu_perf_cfg sm6115_perf_data = {
 
 const struct dpu_mdss_cfg dpu_sm6115_cfg = {
 	.caps = &sm6115_dpu_caps,
-	.ubwc = &sm6115_ubwc_cfg,
 	.mdp_count = ARRAY_SIZE(sm6115_mdp),
 	.mdp = sm6115_mdp,
 	.ctl_count = ARRAY_SIZE(sm6115_ctl),
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 c9003dcc1a59..d69a5e12608d 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
@@ -16,10 +16,6 @@ static const struct dpu_caps qcm2290_dpu_caps = {
 	.pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
 };
 
-static const struct dpu_ubwc_cfg qcm2290_ubwc_cfg = {
-	.highest_bank_bit = 0x2,
-};
-
 static const struct dpu_mdp_cfg qcm2290_mdp[] = {
 	{
 	.name = "top_0", .id = MDP_TOP,
@@ -91,7 +87,6 @@ static const struct dpu_perf_cfg qcm2290_perf_data = {
 
 const struct dpu_mdss_cfg dpu_qcm2290_cfg = {
 	.caps = &qcm2290_dpu_caps,
-	.ubwc = &qcm2290_ubwc_cfg,
 	.mdp_count = ARRAY_SIZE(qcm2290_mdp),
 	.mdp = qcm2290_mdp,
 	.ctl_count = ARRAY_SIZE(qcm2290_ctl),
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 4f6a965bcd90..657593099c17 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
@@ -19,11 +19,6 @@ static const struct dpu_caps sm8350_dpu_caps = {
 	.pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
 };
 
-static const struct dpu_ubwc_cfg sm8350_ubwc_cfg = {
-	.ubwc_version = DPU_HW_UBWC_VER_40,
-	.highest_bank_bit = 0x3, /* TODO: 2 for LP_DDR4 */
-};
-
 static const struct dpu_mdp_cfg sm8350_mdp[] = {
 	{
 	.name = "top_0", .id = MDP_TOP,
@@ -192,7 +187,6 @@ static const struct dpu_perf_cfg sm8350_perf_data = {
 
 const struct dpu_mdss_cfg dpu_sm8350_cfg = {
 	.caps = &sm8350_dpu_caps,
-	.ubwc = &sm8350_ubwc_cfg,
 	.mdp_count = ARRAY_SIZE(sm8350_mdp),
 	.mdp = sm8350_mdp,
 	.ctl_count = ARRAY_SIZE(sm8350_ctl),
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 6b2c7eae71d9..140b6aff1741 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
@@ -17,12 +17,6 @@ static const struct dpu_caps sc7280_dpu_caps = {
 	.pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
 };
 
-static const struct dpu_ubwc_cfg sc7280_ubwc_cfg = {
-	.ubwc_version = DPU_HW_UBWC_VER_30,
-	.highest_bank_bit = 0x1,
-	.ubwc_swizzle = 0x6,
-};
-
 static const struct dpu_mdp_cfg sc7280_mdp[] = {
 	{
 	.name = "top_0", .id = MDP_TOP,
@@ -129,7 +123,6 @@ static const struct dpu_perf_cfg sc7280_perf_data = {
 
 const struct dpu_mdss_cfg dpu_sc7280_cfg = {
 	.caps = &sc7280_dpu_caps,
-	.ubwc = &sc7280_ubwc_cfg,
 	.mdp_count = ARRAY_SIZE(sc7280_mdp),
 	.mdp = sc7280_mdp,
 	.ctl_count = ARRAY_SIZE(sc7280_ctl),
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 706d0f13b598..b215dddf7a5a 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
@@ -19,12 +19,6 @@ static const struct dpu_caps sc8280xp_dpu_caps = {
 	.pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
 };
 
-static const struct dpu_ubwc_cfg sc8280xp_ubwc_cfg = {
-	.ubwc_version = DPU_HW_UBWC_VER_40,
-	.highest_bank_bit = 2,
-	.ubwc_swizzle = 6,
-};
-
 static const struct dpu_mdp_cfg sc8280xp_mdp[] = {
 	{
 	.name = "top_0", .id = MDP_TOP,
@@ -183,7 +177,6 @@ static const struct dpu_perf_cfg sc8280xp_perf_data = {
 
 const struct dpu_mdss_cfg dpu_sc8280xp_cfg = {
 	.caps = &sc8280xp_dpu_caps,
-	.ubwc = &sc8280xp_ubwc_cfg,
 	.mdp_count = ARRAY_SIZE(sc8280xp_mdp),
 	.mdp = sc8280xp_mdp,
 	.ctl_count = ARRAY_SIZE(sc8280xp_ctl),
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 4ecb3df5cbc0..d4f58852fb54 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
@@ -19,12 +19,6 @@ static const struct dpu_caps sm8450_dpu_caps = {
 	.pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
 };
 
-static const struct dpu_ubwc_cfg sm8450_ubwc_cfg = {
-	.ubwc_version = DPU_HW_UBWC_VER_40,
-	.highest_bank_bit = 0x3, /* TODO: 2 for LP_DDR4 */
-	.ubwc_swizzle = 0x6,
-};
-
 static const struct dpu_mdp_cfg sm8450_mdp[] = {
 	{
 	.name = "top_0", .id = MDP_TOP,
@@ -200,7 +194,6 @@ static const struct dpu_perf_cfg sm8450_perf_data = {
 
 const struct dpu_mdss_cfg dpu_sm8450_cfg = {
 	.caps = &sm8450_dpu_caps,
-	.ubwc = &sm8450_ubwc_cfg,
 	.mdp_count = ARRAY_SIZE(sm8450_mdp),
 	.mdp = sm8450_mdp,
 	.ctl_count = ARRAY_SIZE(sm8450_ctl),
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 d0ab351b6a8b..1b446c6052a4 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
@@ -19,11 +19,6 @@ static const struct dpu_caps sm8550_dpu_caps = {
 	.pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
 };
 
-static const struct dpu_ubwc_cfg sm8550_ubwc_cfg = {
-	.ubwc_version = DPU_HW_UBWC_VER_40,
-	.highest_bank_bit = 0x3, /* TODO: 2 for LP_DDR4 */
-};
-
 static const struct dpu_mdp_cfg sm8550_mdp[] = {
 	{
 	.name = "top_0", .id = MDP_TOP,
@@ -205,7 +200,6 @@ static const struct dpu_perf_cfg sm8550_perf_data = {
 
 const struct dpu_mdss_cfg dpu_sm8550_cfg = {
 	.caps = &sm8550_dpu_caps,
-	.ubwc = &sm8550_ubwc_cfg,
 	.mdp_count = ARRAY_SIZE(sm8550_mdp),
 	.mdp = sm8550_mdp,
 	.ctl_count = ARRAY_SIZE(sm8550_ctl),
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 71584cd56fd7..d5088ee86b85 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
@@ -506,19 +506,6 @@ struct dpu_mdp_cfg {
 	struct dpu_clk_ctrl_reg clk_ctrls[DPU_CLK_CTRL_MAX];
 };
 
-/**
- * struct dpu_ubwc_cfg - UBWC and memory configuration
- *
- * @ubwc_version       UBWC feature version (0x0 for not supported)
- * @highest_bank_bit:  UBWC parameter
- * @ubwc_swizzle:      ubwc default swizzle setting
- */
-struct dpu_ubwc_cfg {
-	u32 ubwc_version;
-	u32 highest_bank_bit;
-	u32 ubwc_swizzle;
-};
-
 /* struct dpu_ctl_cfg : MDP CTL instance info
  * @id:                index identifying this block
  * @base:              register base offset to mdss
@@ -818,8 +805,6 @@ struct dpu_perf_cfg {
 struct dpu_mdss_cfg {
 	const struct dpu_caps *caps;
 
-	const struct dpu_ubwc_cfg *ubwc;
-
 	u32 mdp_count;
 	const struct dpu_mdp_cfg *mdp;
 
-- 
2.39.2

Re: [PATCH 0/6] drm/msm/dpu: use UBWC data from MDSS driver

[Steev Klimaszewski]

Hi Dmitry

On Sun, May 21, 2023 at 12:28 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> Both DPU and MDSS programming requires knowledge of some of UBWC
> parameters. This results in duplication of UBWC data between MDSS and
> DPU drivers. To remove such duplication and make the driver more
> error-prone, export respective configuration from the MDSS driver and
> make DPU use it, instead of bundling a copy of such data.
>

Surely you mean less error prone?

> Dmitry Baryshkov (6):
>   drm/msm/mdss: correct UBWC programming for SM8550
>   drm/msm/mdss: rename ubwc_version to ubwc_enc_version
>   drm/msm/mdss: export UBWC data
>   drm/msm/mdss: populate missing data
>   drm/msm/dpu: use MDSS data for programming SSPP
>   drm/msm/dpu: drop UBWC configuration
>
>  .../msm/disp/dpu1/catalog/dpu_3_0_msm8998.h   |  6 --
>  .../msm/disp/dpu1/catalog/dpu_4_0_sdm845.h    |  6 --
>  .../msm/disp/dpu1/catalog/dpu_5_0_sm8150.h    |  6 --
>  .../msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h   |  6 --
>  .../msm/disp/dpu1/catalog/dpu_6_0_sm8250.h    |  7 --
>  .../msm/disp/dpu1/catalog/dpu_6_2_sc7180.h    |  6 --
>  .../msm/disp/dpu1/catalog/dpu_6_3_sm6115.h    |  7 --
>  .../msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h   |  5 --
>  .../msm/disp/dpu1/catalog/dpu_7_0_sm8350.h    |  6 --
>  .../msm/disp/dpu1/catalog/dpu_7_2_sc7280.h    |  7 --
>  .../msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h  |  7 --
>  .../msm/disp/dpu1/catalog/dpu_8_1_sm8450.h    |  7 --
>  .../msm/disp/dpu1/catalog/dpu_9_0_sm8550.h    |  6 --
>  .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h    | 15 ----
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c   | 18 ++--
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h   |  7 +-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c       | 16 +++-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h       |  1 +
>  drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c        |  3 +-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h        |  2 +
>  drivers/gpu/drm/msm/msm_mdss.c                | 90 ++++++++++++-------
>  drivers/gpu/drm/msm/msm_mdss.h                | 27 ++++++
>  22 files changed, 122 insertions(+), 139 deletions(-)
>  create mode 100644 drivers/gpu/drm/msm/msm_mdss.h
>
> --
> 2.39.2
>

Re: [PATCH 0/6] drm/msm/dpu: use UBWC data from MDSS driver

[Dmitry Baryshkov]

On 22/05/2023 00:50, Steev Klimaszewski wrote:
> Hi Dmitry
> 
> On Sun, May 21, 2023 at 12:28 PM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
>>
>> Both DPU and MDSS programming requires knowledge of some of UBWC
>> parameters. This results in duplication of UBWC data between MDSS and
>> DPU drivers. To remove such duplication and make the driver more
>> error-prone, export respective configuration from the MDSS driver and
>> make DPU use it, instead of bundling a copy of such data.
>>
> 
> Surely you mean less error prone?

Yes, of course!

> 
>> Dmitry Baryshkov (6):
>>    drm/msm/mdss: correct UBWC programming for SM8550
>>    drm/msm/mdss: rename ubwc_version to ubwc_enc_version
>>    drm/msm/mdss: export UBWC data
>>    drm/msm/mdss: populate missing data
>>    drm/msm/dpu: use MDSS data for programming SSPP
>>    drm/msm/dpu: drop UBWC configuration
>>
>>   .../msm/disp/dpu1/catalog/dpu_3_0_msm8998.h   |  6 --
>>   .../msm/disp/dpu1/catalog/dpu_4_0_sdm845.h    |  6 --
>>   .../msm/disp/dpu1/catalog/dpu_5_0_sm8150.h    |  6 --
>>   .../msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h   |  6 --
>>   .../msm/disp/dpu1/catalog/dpu_6_0_sm8250.h    |  7 --
>>   .../msm/disp/dpu1/catalog/dpu_6_2_sc7180.h    |  6 --
>>   .../msm/disp/dpu1/catalog/dpu_6_3_sm6115.h    |  7 --
>>   .../msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h   |  5 --
>>   .../msm/disp/dpu1/catalog/dpu_7_0_sm8350.h    |  6 --
>>   .../msm/disp/dpu1/catalog/dpu_7_2_sc7280.h    |  7 --
>>   .../msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h  |  7 --
>>   .../msm/disp/dpu1/catalog/dpu_8_1_sm8450.h    |  7 --
>>   .../msm/disp/dpu1/catalog/dpu_9_0_sm8550.h    |  6 --
>>   .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h    | 15 ----
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c   | 18 ++--
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h   |  7 +-
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c       | 16 +++-
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h       |  1 +
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c        |  3 +-
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h        |  2 +
>>   drivers/gpu/drm/msm/msm_mdss.c                | 90 ++++++++++++-------
>>   drivers/gpu/drm/msm/msm_mdss.h                | 27 ++++++
>>   22 files changed, 122 insertions(+), 139 deletions(-)
>>   create mode 100644 drivers/gpu/drm/msm/msm_mdss.h
>>
>> --
>> 2.39.2
>>

-- 
With best wishes
Dmitry

Re: [PATCH 2/6] drm/msm/mdss: rename ubwc_version to ubwc_enc_version

[Abhinav Kumar]

On 5/21/2023 10:10 AM, Dmitry Baryshkov wrote:
> Rename the ubwc_version field to ubwc_enc_version, it denotes the
> version of the UBWC encoder, not the "UBWC version".
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/msm_mdss.c | 26 +++++++++++++-------------
>   1 file changed, 13 insertions(+), 13 deletions(-)
> 

Yes, this is the encoder version

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

Re: [PATCH 3/6] drm/msm/mdss: export UBWC data

[Abhinav Kumar]

On 5/21/2023 10:10 AM, Dmitry Baryshkov wrote:
> DPU programming requires knowledge of some of UBWC parameters. This
> results in duplication of UBWC data between MDSS and DPU drivers. Export
> the required data from MDSS driver.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/msm_mdss.c | 30 +++++++++++++-----------------
>   drivers/gpu/drm/msm/msm_mdss.h | 27 +++++++++++++++++++++++++++
>   2 files changed, 40 insertions(+), 17 deletions(-)
>   create mode 100644 drivers/gpu/drm/msm/msm_mdss.h
> 
> diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
> index d1e57099b517..ed836c659688 100644
> --- a/drivers/gpu/drm/msm/msm_mdss.c
> +++ b/drivers/gpu/drm/msm/msm_mdss.c
> @@ -13,7 +13,7 @@
>   #include <linux/pm_runtime.h>
>   #include <linux/reset.h>
>   
> -#include "msm_drv.h"
> +#include "msm_mdss.h"
>   #include "msm_kms.h"
>   
>   #define HW_REV				0x0
> @@ -26,16 +26,6 @@
>   
>   #define MIN_IB_BW	400000000UL /* Min ib vote 400MB */
>   
> -struct msm_mdss_data {
> -	u32 ubwc_enc_version;
> -	/* can be read from register 0x58 */
> -	u32 ubwc_dec_version;
> -	u32 ubwc_swizzle;
> -	u32 ubwc_static;
> -	u32 highest_bank_bit;
> -	u32 macrotile_mode;
> -};
> -
>   struct msm_mdss {
>   	struct device *dev;
>   
> @@ -185,12 +175,6 @@ static int _msm_mdss_irq_domain_add(struct msm_mdss *msm_mdss)
>   	return 0;
>   }
>   
> -#define UBWC_1_0 0x10000000
> -#define UBWC_2_0 0x20000000
> -#define UBWC_3_0 0x30000000
> -#define UBWC_4_0 0x40000000
> -#define UBWC_4_3 0x40030000
> -
>   static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss)
>   {
>   	const struct msm_mdss_data *data = msm_mdss->mdss_data;
> @@ -236,6 +220,18 @@ static void msm_mdss_setup_ubwc_dec_40(struct msm_mdss *msm_mdss)
>   	}
>   }
>   
> +const struct msm_mdss_data *msm_mdss_get_mdss_data(struct device *dev)
> +{
> +	struct msm_mdss *mdss;
> +
> +	if (!dev)
> +		return ERR_PTR(-EINVAL);
> +
> +	mdss = dev_get_drvdata(dev);
> +
> +	return mdss->mdss_data;
> +}
> +
>   static int msm_mdss_enable(struct msm_mdss *msm_mdss)
>   {
>   	int ret;
> diff --git a/drivers/gpu/drm/msm/msm_mdss.h b/drivers/gpu/drm/msm/msm_mdss.h
> new file mode 100644
> index 000000000000..02bbab42adbc
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/msm_mdss.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) 2018, The Linux Foundation
> + */

Fix the copyright year .

Apart from that,

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

Re: [PATCH 3/6] drm/msm/mdss: export UBWC data

[Dmitry Baryshkov]

On Thu, 27 Jul 2023 at 00:21, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 5/21/2023 10:10 AM, Dmitry Baryshkov wrote:
> > DPU programming requires knowledge of some of UBWC parameters. This
> > results in duplication of UBWC data between MDSS and DPU drivers. Export
> > the required data from MDSS driver.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >   drivers/gpu/drm/msm/msm_mdss.c | 30 +++++++++++++-----------------
> >   drivers/gpu/drm/msm/msm_mdss.h | 27 +++++++++++++++++++++++++++
> >   2 files changed, 40 insertions(+), 17 deletions(-)
> >   create mode 100644 drivers/gpu/drm/msm/msm_mdss.h
> >
> > diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
> > index d1e57099b517..ed836c659688 100644
> > --- a/drivers/gpu/drm/msm/msm_mdss.c
> > +++ b/drivers/gpu/drm/msm/msm_mdss.c
> > @@ -13,7 +13,7 @@
> >   #include <linux/pm_runtime.h>
> >   #include <linux/reset.h>
> >
> > -#include "msm_drv.h"
> > +#include "msm_mdss.h"
> >   #include "msm_kms.h"
> >
> >   #define HW_REV                              0x0
> > @@ -26,16 +26,6 @@
> >
> >   #define MIN_IB_BW   400000000UL /* Min ib vote 400MB */
> >
> > -struct msm_mdss_data {
> > -     u32 ubwc_enc_version;
> > -     /* can be read from register 0x58 */
> > -     u32 ubwc_dec_version;
> > -     u32 ubwc_swizzle;
> > -     u32 ubwc_static;
> > -     u32 highest_bank_bit;
> > -     u32 macrotile_mode;
> > -};
> > -
> >   struct msm_mdss {
> >       struct device *dev;
> >
> > @@ -185,12 +175,6 @@ static int _msm_mdss_irq_domain_add(struct msm_mdss *msm_mdss)
> >       return 0;
> >   }
> >
> > -#define UBWC_1_0 0x10000000
> > -#define UBWC_2_0 0x20000000
> > -#define UBWC_3_0 0x30000000
> > -#define UBWC_4_0 0x40000000
> > -#define UBWC_4_3 0x40030000
> > -
> >   static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss)
> >   {
> >       const struct msm_mdss_data *data = msm_mdss->mdss_data;
> > @@ -236,6 +220,18 @@ static void msm_mdss_setup_ubwc_dec_40(struct msm_mdss *msm_mdss)
> >       }
> >   }
> >
> > +const struct msm_mdss_data *msm_mdss_get_mdss_data(struct device *dev)
> > +{
> > +     struct msm_mdss *mdss;
> > +
> > +     if (!dev)
> > +             return ERR_PTR(-EINVAL);
> > +
> > +     mdss = dev_get_drvdata(dev);
> > +
> > +     return mdss->mdss_data;
> > +}
> > +
> >   static int msm_mdss_enable(struct msm_mdss *msm_mdss)
> >   {
> >       int ret;
> > diff --git a/drivers/gpu/drm/msm/msm_mdss.h b/drivers/gpu/drm/msm/msm_mdss.h
> > new file mode 100644
> > index 000000000000..02bbab42adbc
> > --- /dev/null
> > +++ b/drivers/gpu/drm/msm/msm_mdss.h
> > @@ -0,0 +1,27 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Copyright (c) 2018, The Linux Foundation
> > + */
>
> Fix the copyright year .

No, the copyright year is correct. The data was moved from the c file
(which has this copyright) and there are no copyrightable additions.

>
> Apart from that,
>
> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>



-- 
With best wishes
Dmitry

Re: [PATCH 4/6] drm/msm/mdss: populate missing data

[Abhinav Kumar]

On 5/21/2023 10:10 AM, Dmitry Baryshkov wrote:
> As we are going to use MDSS data for DPU programming, populate missing
> MDSS data. The UBWC 1.0 and no UBWC cases do not require MDSS
> programming, so skip them.
> 

Can you pls point me to the downstream references you used for msm8998?

Was that just taken from catalog? If so I would ask for the reference 
for the catalog too.

As per the register the decoder version is 0x0 and not 1.

Even encoder version is 0 from what i see and not 1. Thats why a 
dec_version of UBWC_1_0 is not doing anything i assume.

Some additional questions:

1) Does the whole chunk in dpu_hw_sspp_setup_format() which handles ubwc 
programming need to be protected by if (ctx->ubwc) now ?

2) The values of UBWC_x_x dont match the values of DPU_HW_UBWC_VER_xx.
What was the reason for the catalog to go with DPU_HW_UBWC_VER_xx in the 
catalog for the encoder version in the first place? Because looking at 
the registers UBWC_x_x is the correct value.


> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/msm_mdss.c | 21 +++++++++++++++++++--
>   1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
> index ed836c659688..9bb7be4b9ebb 100644
> --- a/drivers/gpu/drm/msm/msm_mdss.c
> +++ b/drivers/gpu/drm/msm/msm_mdss.c
> @@ -264,6 +264,10 @@ static int msm_mdss_enable(struct msm_mdss *msm_mdss)
>   	 * UBWC_n and the rest of params comes from hw data.
>   	 */
>   	switch (msm_mdss->mdss_data->ubwc_dec_version) {
> +	case 0: /* no UBWC */
> +	case UBWC_1_0:
> +		/* do nothing */
> +		break;
>   	case UBWC_2_0:
>   		msm_mdss_setup_ubwc_dec_20(msm_mdss);
>   		break;
> @@ -502,10 +506,22 @@ static int mdss_remove(struct platform_device *pdev)
>   	return 0;
>   }
>   
> +static const struct msm_mdss_data msm8998_data = {
> +	.ubwc_enc_version = UBWC_1_0,
> +	.ubwc_dec_version = UBWC_1_0,
> +	.highest_bank_bit = 1,
> +};
> +
> +static const struct msm_mdss_data qcm2290_data = {
> +	/* no UBWC */
> +	.highest_bank_bit = 0x2,
> +};
> +
>   static const struct msm_mdss_data sc7180_data = {
>   	.ubwc_enc_version = UBWC_2_0,
>   	.ubwc_dec_version = UBWC_2_0,
>   	.ubwc_static = 0x1e,
> +	.highest_bank_bit = 0x3,
>   };
>   
>   static const struct msm_mdss_data sc7280_data = {
> @@ -550,6 +566,7 @@ static const struct msm_mdss_data sm6115_data = {
>   	.ubwc_dec_version = UBWC_2_0,
>   	.ubwc_swizzle = 7,
>   	.ubwc_static = 0x11f,
> +	.highest_bank_bit = 0x1,
>   };
>   
>   static const struct msm_mdss_data sm8250_data = {
> @@ -574,8 +591,8 @@ static const struct msm_mdss_data sm8550_data = {
>   
>   static const struct of_device_id mdss_dt_match[] = {
>   	{ .compatible = "qcom,mdss" },
> -	{ .compatible = "qcom,msm8998-mdss" },
> -	{ .compatible = "qcom,qcm2290-mdss" },
> +	{ .compatible = "qcom,msm8998-mdss", .data = &msm8998_data },
> +	{ .compatible = "qcom,qcm2290-mdss", .data = &qcm2290_data },
>   	{ .compatible = "qcom,sdm845-mdss", .data = &sdm845_data },
>   	{ .compatible = "qcom,sc7180-mdss", .data = &sc7180_data },
>   	{ .compatible = "qcom,sc7280-mdss", .data = &sc7280_data },

Re: [PATCH 4/6] drm/msm/mdss: populate missing data

[Dmitry Baryshkov]

On Thu, 27 Jul 2023 at 01:30, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 5/21/2023 10:10 AM, Dmitry Baryshkov wrote:
> > As we are going to use MDSS data for DPU programming, populate missing
> > MDSS data. The UBWC 1.0 and no UBWC cases do not require MDSS
> > programming, so skip them.
> >
>
> Can you pls point me to the downstream references you used for msm8998?

msm-3.18, drivers/video/msm/mdss/mdss_mdp.c

See the function mdss_mdp_hw_rev_caps_init(). It sets has_ubwc for MDP
1.07 (msm8996), 1.14 (msm8937) / 1.16  (msm8953) and 3.0 (msm8998).

> Was that just taken from catalog? If so I would ask for the reference
> for the catalog too.
>
> As per the register the decoder version is 0x0 and not 1.
>
> Even encoder version is 0 from what i see and not 1. Thats why a
> dec_version of UBWC_1_0 is not doing anything i assume.
>
> Some additional questions:
>
> 1) Does the whole chunk in dpu_hw_sspp_setup_format() which handles ubwc
> programming need to be protected by if (ctx->ubwc) now ?

It's hard to discuss the question which is irrelevant for this patch.
Nevertheless, yes, it needs to be protected because e.g. qcm2290
doesn't have UBWC support.

> 2) The values of UBWC_x_x dont match the values of DPU_HW_UBWC_VER_xx.
> What was the reason for the catalog to go with DPU_HW_UBWC_VER_xx in the
> catalog for the encoder version in the first place? Because looking at
> the registers UBWC_x_x is the correct value.

Huh. This definitely should be asked next to the code that you wish to
discuss. The DPU_HW_UBWC_VER_xx values come from the first DPU
revision.

>
>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >   drivers/gpu/drm/msm/msm_mdss.c | 21 +++++++++++++++++++--
> >   1 file changed, 19 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
> > index ed836c659688..9bb7be4b9ebb 100644
> > --- a/drivers/gpu/drm/msm/msm_mdss.c
> > +++ b/drivers/gpu/drm/msm/msm_mdss.c
> > @@ -264,6 +264,10 @@ static int msm_mdss_enable(struct msm_mdss *msm_mdss)
> >        * UBWC_n and the rest of params comes from hw data.
> >        */
> >       switch (msm_mdss->mdss_data->ubwc_dec_version) {
> > +     case 0: /* no UBWC */
> > +     case UBWC_1_0:
> > +             /* do nothing */
> > +             break;
> >       case UBWC_2_0:
> >               msm_mdss_setup_ubwc_dec_20(msm_mdss);
> >               break;
> > @@ -502,10 +506,22 @@ static int mdss_remove(struct platform_device *pdev)
> >       return 0;
> >   }
> >
> > +static const struct msm_mdss_data msm8998_data = {
> > +     .ubwc_enc_version = UBWC_1_0,
> > +     .ubwc_dec_version = UBWC_1_0,
> > +     .highest_bank_bit = 1,
> > +};
> > +
> > +static const struct msm_mdss_data qcm2290_data = {
> > +     /* no UBWC */
> > +     .highest_bank_bit = 0x2,
> > +};
> > +
> >   static const struct msm_mdss_data sc7180_data = {
> >       .ubwc_enc_version = UBWC_2_0,
> >       .ubwc_dec_version = UBWC_2_0,
> >       .ubwc_static = 0x1e,
> > +     .highest_bank_bit = 0x3,
> >   };
> >
> >   static const struct msm_mdss_data sc7280_data = {
> > @@ -550,6 +566,7 @@ static const struct msm_mdss_data sm6115_data = {
> >       .ubwc_dec_version = UBWC_2_0,
> >       .ubwc_swizzle = 7,
> >       .ubwc_static = 0x11f,
> > +     .highest_bank_bit = 0x1,
> >   };
> >
> >   static const struct msm_mdss_data sm8250_data = {
> > @@ -574,8 +591,8 @@ static const struct msm_mdss_data sm8550_data = {
> >
> >   static const struct of_device_id mdss_dt_match[] = {
> >       { .compatible = "qcom,mdss" },
> > -     { .compatible = "qcom,msm8998-mdss" },
> > -     { .compatible = "qcom,qcm2290-mdss" },
> > +     { .compatible = "qcom,msm8998-mdss", .data = &msm8998_data },
> > +     { .compatible = "qcom,qcm2290-mdss", .data = &qcm2290_data },
> >       { .compatible = "qcom,sdm845-mdss", .data = &sdm845_data },
> >       { .compatible = "qcom,sc7180-mdss", .data = &sc7180_data },
> >       { .compatible = "qcom,sc7280-mdss", .data = &sc7280_data },



-- 
With best wishes
Dmitry

Re: [PATCH 4/6] drm/msm/mdss: populate missing data

[Abhinav Kumar]

On 7/26/2023 3:58 PM, Dmitry Baryshkov wrote:
> On Thu, 27 Jul 2023 at 01:30, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 5/21/2023 10:10 AM, Dmitry Baryshkov wrote:
>>> As we are going to use MDSS data for DPU programming, populate missing
>>> MDSS data. The UBWC 1.0 and no UBWC cases do not require MDSS
>>> programming, so skip them.
>>>
>>
>> Can you pls point me to the downstream references you used for msm8998?
> 
> msm-3.18, drivers/video/msm/mdss/mdss_mdp.c
> 
> See the function mdss_mdp_hw_rev_caps_init(). It sets has_ubwc for MDP
> 1.07 (msm8996), 1.14 (msm8937) / 1.16  (msm8953) and 3.0 (msm8998).
> 

It sets has_ubwc but I still cannot locate where it says version is 1.0.
Because the 0x58 register reads 0 and not 1 for msm8998.

>> Was that just taken from catalog? If so I would ask for the reference
>> for the catalog too.
>>
>> As per the register the decoder version is 0x0 and not 1.
>>
>> Even encoder version is 0 from what i see and not 1. Thats why a
>> dec_version of UBWC_1_0 is not doing anything i assume.
>>
>> Some additional questions:
>>
>> 1) Does the whole chunk in dpu_hw_sspp_setup_format() which handles ubwc
>> programming need to be protected by if (ctx->ubwc) now ?
> 
> It's hard to discuss the question which is irrelevant for this patch.
> Nevertheless, yes, it needs to be protected because e.g. qcm2290
> doesn't have UBWC support.
> 

Sorry but your commit text made me look into this function and wonder 
now what happens to that code. But I will continue this in this next 
patch so that you can see the code and the question together.

>> 2) The values of UBWC_x_x dont match the values of DPU_HW_UBWC_VER_xx.
>> What was the reason for the catalog to go with DPU_HW_UBWC_VER_xx in the
>> catalog for the encoder version in the first place? Because looking at
>> the registers UBWC_x_x is the correct value.
> 
> Huh. This definitely should be asked next to the code that you wish to
> discuss. The DPU_HW_UBWC_VER_xx values come from the first DPU
> revision.
> 

Sure, so again the next patch.

>>
>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>    drivers/gpu/drm/msm/msm_mdss.c | 21 +++++++++++++++++++--
>>>    1 file changed, 19 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
>>> index ed836c659688..9bb7be4b9ebb 100644
>>> --- a/drivers/gpu/drm/msm/msm_mdss.c
>>> +++ b/drivers/gpu/drm/msm/msm_mdss.c
>>> @@ -264,6 +264,10 @@ static int msm_mdss_enable(struct msm_mdss *msm_mdss)
>>>         * UBWC_n and the rest of params comes from hw data.
>>>         */
>>>        switch (msm_mdss->mdss_data->ubwc_dec_version) {
>>> +     case 0: /* no UBWC */
>>> +     case UBWC_1_0:
>>> +             /* do nothing */
>>> +             break;
>>>        case UBWC_2_0:
>>>                msm_mdss_setup_ubwc_dec_20(msm_mdss);
>>>                break;
>>> @@ -502,10 +506,22 @@ static int mdss_remove(struct platform_device *pdev)
>>>        return 0;
>>>    }
>>>
>>> +static const struct msm_mdss_data msm8998_data = {
>>> +     .ubwc_enc_version = UBWC_1_0,
>>> +     .ubwc_dec_version = UBWC_1_0,
>>> +     .highest_bank_bit = 1,
>>> +};
>>> +
>>> +static const struct msm_mdss_data qcm2290_data = {
>>> +     /* no UBWC */
>>> +     .highest_bank_bit = 0x2,
>>> +};
>>> +
>>>    static const struct msm_mdss_data sc7180_data = {
>>>        .ubwc_enc_version = UBWC_2_0,
>>>        .ubwc_dec_version = UBWC_2_0,
>>>        .ubwc_static = 0x1e,
>>> +     .highest_bank_bit = 0x3,
>>>    };
>>>
>>>    static const struct msm_mdss_data sc7280_data = {
>>> @@ -550,6 +566,7 @@ static const struct msm_mdss_data sm6115_data = {
>>>        .ubwc_dec_version = UBWC_2_0,
>>>        .ubwc_swizzle = 7,
>>>        .ubwc_static = 0x11f,
>>> +     .highest_bank_bit = 0x1,
>>>    };
>>>
>>>    static const struct msm_mdss_data sm8250_data = {
>>> @@ -574,8 +591,8 @@ static const struct msm_mdss_data sm8550_data = {
>>>
>>>    static const struct of_device_id mdss_dt_match[] = {
>>>        { .compatible = "qcom,mdss" },
>>> -     { .compatible = "qcom,msm8998-mdss" },
>>> -     { .compatible = "qcom,qcm2290-mdss" },
>>> +     { .compatible = "qcom,msm8998-mdss", .data = &msm8998_data },
>>> +     { .compatible = "qcom,qcm2290-mdss", .data = &qcm2290_data },
>>>        { .compatible = "qcom,sdm845-mdss", .data = &sdm845_data },
>>>        { .compatible = "qcom,sc7180-mdss", .data = &sc7180_data },
>>>        { .compatible = "qcom,sc7280-mdss", .data = &sc7280_data },
> 
> 
> 

Re: [PATCH 5/6] drm/msm/dpu: use MDSS data for programming SSPP

[Abhinav Kumar]

On 5/21/2023 10:10 AM, Dmitry Baryshkov wrote:
> Switch to using data from MDSS driver to program the SSPP fetch and UBWC
> configuration.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 18 +++++++++++-------
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h |  7 +++++--
>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     | 16 +++++++++++++++-
>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h     |  1 +
>   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c      |  3 ++-
>   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h      |  2 ++
>   6 files changed, 36 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
> index cf70a9bd1034..bfd82c2921af 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
> @@ -8,6 +8,8 @@
>   #include "dpu_hw_sspp.h"
>   #include "dpu_kms.h"
>   
> +#include "msm_mdss.h"
> +
>   #include <drm/drm_file.h>
>   
>   #define DPU_FETCH_CONFIG_RESET_VALUE   0x00000087
> @@ -308,26 +310,26 @@ static void dpu_hw_sspp_setup_format(struct dpu_sw_pipe *pipe,
>   		DPU_REG_WRITE(c, SSPP_FETCH_CONFIG,
>   			DPU_FETCH_CONFIG_RESET_VALUE |
>   			ctx->ubwc->highest_bank_bit << 18);

Does this needs to be protected with if ctx->ubwc check?

> -		switch (ctx->ubwc->ubwc_version) {
> -		case DPU_HW_UBWC_VER_10:
> +		switch (ctx->ubwc->ubwc_enc_version) {
> +		case UBWC_1_0:

The values of UBWC_x_x dont match the values of DPU_HW_UBWC_VER_xx.
What was the reason for the catalog to go with DPU_HW_UBWC_VER_xx in the 
catalog for the encoder version in the first place? Because looking at 
the registers UBWC_x_x is the correct value.

If we cannot find the reason why, it should be noted in the commit text 
that the values we are using did change.

>   			fast_clear = fmt->alpha_enable ? BIT(31) : 0;
>   			DPU_REG_WRITE(c, SSPP_UBWC_STATIC_CTRL,
>   					fast_clear | (ctx->ubwc->ubwc_swizzle & 0x1) |
>   					BIT(8) |
>   					(ctx->ubwc->highest_bank_bit << 4));
>   			break;
> -		case DPU_HW_UBWC_VER_20:
> +		case UBWC_2_0:
>   			fast_clear = fmt->alpha_enable ? BIT(31) : 0;
>   			DPU_REG_WRITE(c, SSPP_UBWC_STATIC_CTRL,
>   					fast_clear | (ctx->ubwc->ubwc_swizzle) |
>   					(ctx->ubwc->highest_bank_bit << 4));
>   			break;
> -		case DPU_HW_UBWC_VER_30:
> +		case UBWC_3_0:
>   			DPU_REG_WRITE(c, SSPP_UBWC_STATIC_CTRL,
>   					BIT(30) | (ctx->ubwc->ubwc_swizzle) |
>   					(ctx->ubwc->highest_bank_bit << 4));
>   			break;
> -		case DPU_HW_UBWC_VER_40:
> +		case UBWC_4_0:
>   			DPU_REG_WRITE(c, SSPP_UBWC_STATIC_CTRL,
>   					DPU_FORMAT_IS_YUV(fmt) ? 0 : BIT(30));
>   			break;
> @@ -793,7 +795,9 @@ static const struct dpu_sspp_cfg *_sspp_offset(enum dpu_sspp sspp,
>   }
>   
>   struct dpu_hw_sspp *dpu_hw_sspp_init(enum dpu_sspp idx,
> -		void __iomem *addr, const struct dpu_mdss_cfg *catalog)
> +				     void __iomem *addr,
> +				     const struct dpu_mdss_cfg *catalog,
> +				     const struct msm_mdss_data *mdss_data)
>   {
>   	struct dpu_hw_sspp *hw_pipe;
>   	const struct dpu_sspp_cfg *cfg;
> @@ -813,7 +817,7 @@ struct dpu_hw_sspp *dpu_hw_sspp_init(enum dpu_sspp idx,
>   
>   	/* Assign ops */
>   	hw_pipe->catalog = catalog;
> -	hw_pipe->ubwc = catalog->ubwc;
> +	hw_pipe->ubwc = mdss_data;
>   	hw_pipe->idx = idx;
>   	hw_pipe->cap = cfg;
>   	_setup_layer_ops(hw_pipe, hw_pipe->cap->features);
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
> index 74b98b6b3bc3..8d4ae9d24674 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
> @@ -351,7 +351,7 @@ struct dpu_hw_sspp {
>   	struct dpu_hw_blk base;
>   	struct dpu_hw_blk_reg_map hw;
>   	const struct dpu_mdss_cfg *catalog;
> -	const struct dpu_ubwc_cfg *ubwc;
> +	const struct msm_mdss_data *ubwc;
>   
>   	/* Pipe */
>   	enum dpu_sspp idx;
> @@ -368,9 +368,12 @@ struct dpu_kms;
>    * @idx:  Pipe index for which driver object is required
>    * @addr: Mapped register io address of MDP
>    * @catalog : Pointer to mdss catalog data
> + * @mdss_data: UBWC / MDSS configuration data
>    */
>   struct dpu_hw_sspp *dpu_hw_sspp_init(enum dpu_sspp idx,
> -		void __iomem *addr, const struct dpu_mdss_cfg *catalog);
> +				     void __iomem *addr,
> +				     const struct dpu_mdss_cfg *catalog,
> +				     const struct msm_mdss_data *mdss_data);
>   
>   /**
>    * dpu_hw_sspp_destroy(): Destroys SSPP driver context
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 0e7a68714e9e..a4293dc0b61b 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -22,6 +22,7 @@
>   
>   #include "msm_drv.h"
>   #include "msm_mmu.h"
> +#include "msm_mdss.h"
>   #include "msm_gem.h"
>   #include "disp/msm_disp_snapshot.h"
>   
> @@ -1066,7 +1067,20 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
>   		goto power_error;
>   	}
>   
> -	rc = dpu_rm_init(&dpu_kms->rm, dpu_kms->catalog, dpu_kms->mmio);
> +	dpu_kms->mdss = msm_mdss_get_mdss_data(dpu_kms->pdev->dev.parent);
> +	if (IS_ERR(dpu_kms->mdss)) {
> +		rc = PTR_ERR(dpu_kms->mdss);
> +		DPU_ERROR("failed to get MDSS data: %d\n", rc);
> +		goto power_error;
> +	}
> +
> +	if (!dpu_kms->mdss) {
> +		rc = -EINVAL;
> +		DPU_ERROR("NULL MDSS data\n");
> +		goto power_error;
> +	}
> +
> +	rc = dpu_rm_init(&dpu_kms->rm, dpu_kms->catalog, dpu_kms->mdss, dpu_kms->mmio);
>   	if (rc) {
>   		DPU_ERROR("rm init failed: %d\n", rc);
>   		goto power_error;
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> index aca39a4689f4..797b4ff3e806 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> @@ -69,6 +69,7 @@ struct dpu_kms {
>   	struct msm_kms base;
>   	struct drm_device *dev;
>   	const struct dpu_mdss_cfg *catalog;
> +	const struct msm_mdss_data *mdss;
>   
>   	/* io/register spaces: */
>   	void __iomem *mmio, *vbif[VBIF_MAX], *reg_dma;
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> index f4dda88a73f7..9ee493465c4b 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> @@ -100,6 +100,7 @@ int dpu_rm_destroy(struct dpu_rm *rm)
>   
>   int dpu_rm_init(struct dpu_rm *rm,
>   		const struct dpu_mdss_cfg *cat,
> +		const struct msm_mdss_data *mdss_data,
>   		void __iomem *mmio)
>   {
>   	int rc, i;
> @@ -268,7 +269,7 @@ int dpu_rm_init(struct dpu_rm *rm,
>   			continue;
>   		}
>   
> -		hw = dpu_hw_sspp_init(sspp->id, mmio, cat);
> +		hw = dpu_hw_sspp_init(sspp->id, mmio, cat, mdss_data);
>   		if (IS_ERR(hw)) {
>   			rc = PTR_ERR(hw);
>   			DPU_ERROR("failed sspp object creation: err %d\n", rc);
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> index d62c2edb2460..2b551566cbf4 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> @@ -40,11 +40,13 @@ struct dpu_rm {
>    *	for all HW blocks.
>    * @rm: DPU Resource Manager handle
>    * @cat: Pointer to hardware catalog
> + * @mdss_data: Pointer to MDSS / UBWC configuration
>    * @mmio: mapped register io address of MDP
>    * @Return: 0 on Success otherwise -ERROR
>    */
>   int dpu_rm_init(struct dpu_rm *rm,
>   		const struct dpu_mdss_cfg *cat,
> +		const struct msm_mdss_data *mdss_data,
>   		void __iomem *mmio);
>   
>   /**

Re: [PATCH 6/6] drm/msm/dpu: drop UBWC configuration

[Abhinav Kumar]

On 5/21/2023 10:10 AM, Dmitry Baryshkov wrote:
> As the DPU driver has switched to fetching data from MDSS driver, we can
> now drop the UBWC and highest_bank_bit parts of the DPU hw catalog.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   .../drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h   |  6 ------
>   .../drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h    |  6 ------
>   .../drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h    |  6 ------
>   .../drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h   |  6 ------
>   .../drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h    |  7 -------
>   .../drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h    |  6 ------
>   .../drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h    |  7 -------
>   .../drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h   |  5 -----
>   .../drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h    |  6 ------
>   .../drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h    |  7 -------
>   .../drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h  |  7 -------
>   .../drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h    |  7 -------
>   .../drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h    |  6 ------
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h    | 15 ---------------
>   14 files changed, 97 deletions(-)
> 

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

Re: [PATCH 4/6] drm/msm/mdss: populate missing data

[Dmitry Baryshkov]

On Thu, 27 Jul 2023 at 02:14, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 7/26/2023 3:58 PM, Dmitry Baryshkov wrote:
> > On Thu, 27 Jul 2023 at 01:30, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>
> >>
> >>
> >> On 5/21/2023 10:10 AM, Dmitry Baryshkov wrote:
> >>> As we are going to use MDSS data for DPU programming, populate missing
> >>> MDSS data. The UBWC 1.0 and no UBWC cases do not require MDSS
> >>> programming, so skip them.
> >>>
> >>
> >> Can you pls point me to the downstream references you used for msm8998?
> >
> > msm-3.18, drivers/video/msm/mdss/mdss_mdp.c
> >
> > See the function mdss_mdp_hw_rev_caps_init(). It sets has_ubwc for MDP
> > 1.07 (msm8996), 1.14 (msm8937) / 1.16  (msm8953) and 3.0 (msm8998).
> >
>
> It sets has_ubwc but I still cannot locate where it says version is 1.0.
> Because the 0x58 register reads 0 and not 1 for msm8998.

What would be the version then? 0.0 sounds strange. I'm yet to check
whether UBWC works on 8996 / 8998.

> >> Was that just taken from catalog? If so I would ask for the reference
> >> for the catalog too.
> >>
> >> As per the register the decoder version is 0x0 and not 1.
> >>
> >> Even encoder version is 0 from what i see and not 1. Thats why a
> >> dec_version of UBWC_1_0 is not doing anything i assume.
> >>


-- 
With best wishes
Dmitry

Re: [PATCH 5/6] drm/msm/dpu: use MDSS data for programming SSPP

[Dmitry Baryshkov]

On Thu, 27 Jul 2023 at 02:20, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 5/21/2023 10:10 AM, Dmitry Baryshkov wrote:
> > Switch to using data from MDSS driver to program the SSPP fetch and UBWC
> > configuration.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 18 +++++++++++-------
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h |  7 +++++--
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     | 16 +++++++++++++++-
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h     |  1 +
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c      |  3 ++-
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h      |  2 ++
> >   6 files changed, 36 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
> > index cf70a9bd1034..bfd82c2921af 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
> > @@ -8,6 +8,8 @@
> >   #include "dpu_hw_sspp.h"
> >   #include "dpu_kms.h"
> >
> > +#include "msm_mdss.h"
> > +
> >   #include <drm/drm_file.h>
> >
> >   #define DPU_FETCH_CONFIG_RESET_VALUE   0x00000087
> > @@ -308,26 +310,26 @@ static void dpu_hw_sspp_setup_format(struct dpu_sw_pipe *pipe,
> >               DPU_REG_WRITE(c, SSPP_FETCH_CONFIG,
> >                       DPU_FETCH_CONFIG_RESET_VALUE |
> >                       ctx->ubwc->highest_bank_bit << 18);
>
> Does this needs to be protected with if ctx->ubwc check?

Yes... And it should have been already.

>
> > -             switch (ctx->ubwc->ubwc_version) {
> > -             case DPU_HW_UBWC_VER_10:
> > +             switch (ctx->ubwc->ubwc_enc_version) {
> > +             case UBWC_1_0:
>
> The values of UBWC_x_x dont match the values of DPU_HW_UBWC_VER_xx.
> What was the reason for the catalog to go with DPU_HW_UBWC_VER_xx in the
> catalog for the encoder version in the first place? Because looking at
> the registers UBWC_x_x is the correct value.
>
> If we cannot find the reason why, it should be noted in the commit text
> that the values we are using did change.

Huh? This is just an enum. It isn't a part of uABI, nor it is written
to the hardware.

>
> >                       fast_clear = fmt->alpha_enable ? BIT(31) : 0;
> >                       DPU_REG_WRITE(c, SSPP_UBWC_STATIC_CTRL,
> >                                       fast_clear | (ctx->ubwc->ubwc_swizzle & 0x1) |
> >                                       BIT(8) |
> >                                       (ctx->ubwc->highest_bank_bit << 4));
> >                       break;
> > -             case DPU_HW_UBWC_VER_20:
> > +             case UBWC_2_0:
> >                       fast_clear = fmt->alpha_enable ? BIT(31) : 0;
> >                       DPU_REG_WRITE(c, SSPP_UBWC_STATIC_CTRL,
> >                                       fast_clear | (ctx->ubwc->ubwc_swizzle) |
> >                                       (ctx->ubwc->highest_bank_bit << 4));
> >                       break;
> > -             case DPU_HW_UBWC_VER_30:
> > +             case UBWC_3_0:
> >                       DPU_REG_WRITE(c, SSPP_UBWC_STATIC_CTRL,
> >                                       BIT(30) | (ctx->ubwc->ubwc_swizzle) |
> >                                       (ctx->ubwc->highest_bank_bit << 4));
> >                       break;
> > -             case DPU_HW_UBWC_VER_40:
> > +             case UBWC_4_0:
> >                       DPU_REG_WRITE(c, SSPP_UBWC_STATIC_CTRL,
> >                                       DPU_FORMAT_IS_YUV(fmt) ? 0 : BIT(30));
> >                       break;
> > @@ -793,7 +795,9 @@ static const struct dpu_sspp_cfg *_sspp_offset(enum dpu_sspp sspp,
> >   }
> >
> >   struct dpu_hw_sspp *dpu_hw_sspp_init(enum dpu_sspp idx,
> > -             void __iomem *addr, const struct dpu_mdss_cfg *catalog)
> > +                                  void __iomem *addr,
> > +                                  const struct dpu_mdss_cfg *catalog,
> > +                                  const struct msm_mdss_data *mdss_data)
> >   {
> >       struct dpu_hw_sspp *hw_pipe;
> >       const struct dpu_sspp_cfg *cfg;
> > @@ -813,7 +817,7 @@ struct dpu_hw_sspp *dpu_hw_sspp_init(enum dpu_sspp idx,
> >
> >       /* Assign ops */
> >       hw_pipe->catalog = catalog;
> > -     hw_pipe->ubwc = catalog->ubwc;
> > +     hw_pipe->ubwc = mdss_data;
> >       hw_pipe->idx = idx;
> >       hw_pipe->cap = cfg;
> >       _setup_layer_ops(hw_pipe, hw_pipe->cap->features);
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
> > index 74b98b6b3bc3..8d4ae9d24674 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
> > @@ -351,7 +351,7 @@ struct dpu_hw_sspp {
> >       struct dpu_hw_blk base;
> >       struct dpu_hw_blk_reg_map hw;
> >       const struct dpu_mdss_cfg *catalog;
> > -     const struct dpu_ubwc_cfg *ubwc;
> > +     const struct msm_mdss_data *ubwc;
> >
> >       /* Pipe */
> >       enum dpu_sspp idx;
> > @@ -368,9 +368,12 @@ struct dpu_kms;
> >    * @idx:  Pipe index for which driver object is required
> >    * @addr: Mapped register io address of MDP
> >    * @catalog : Pointer to mdss catalog data
> > + * @mdss_data: UBWC / MDSS configuration data
> >    */
> >   struct dpu_hw_sspp *dpu_hw_sspp_init(enum dpu_sspp idx,
> > -             void __iomem *addr, const struct dpu_mdss_cfg *catalog);
> > +                                  void __iomem *addr,
> > +                                  const struct dpu_mdss_cfg *catalog,
> > +                                  const struct msm_mdss_data *mdss_data);
> >
> >   /**
> >    * dpu_hw_sspp_destroy(): Destroys SSPP driver context
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > index 0e7a68714e9e..a4293dc0b61b 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > @@ -22,6 +22,7 @@
> >
> >   #include "msm_drv.h"
> >   #include "msm_mmu.h"
> > +#include "msm_mdss.h"
> >   #include "msm_gem.h"
> >   #include "disp/msm_disp_snapshot.h"
> >
> > @@ -1066,7 +1067,20 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
> >               goto power_error;
> >       }
> >
> > -     rc = dpu_rm_init(&dpu_kms->rm, dpu_kms->catalog, dpu_kms->mmio);
> > +     dpu_kms->mdss = msm_mdss_get_mdss_data(dpu_kms->pdev->dev.parent);
> > +     if (IS_ERR(dpu_kms->mdss)) {
> > +             rc = PTR_ERR(dpu_kms->mdss);
> > +             DPU_ERROR("failed to get MDSS data: %d\n", rc);
> > +             goto power_error;
> > +     }
> > +
> > +     if (!dpu_kms->mdss) {
> > +             rc = -EINVAL;
> > +             DPU_ERROR("NULL MDSS data\n");
> > +             goto power_error;
> > +     }
> > +
> > +     rc = dpu_rm_init(&dpu_kms->rm, dpu_kms->catalog, dpu_kms->mdss, dpu_kms->mmio);
> >       if (rc) {
> >               DPU_ERROR("rm init failed: %d\n", rc);
> >               goto power_error;
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> > index aca39a4689f4..797b4ff3e806 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> > @@ -69,6 +69,7 @@ struct dpu_kms {
> >       struct msm_kms base;
> >       struct drm_device *dev;
> >       const struct dpu_mdss_cfg *catalog;
> > +     const struct msm_mdss_data *mdss;
> >
> >       /* io/register spaces: */
> >       void __iomem *mmio, *vbif[VBIF_MAX], *reg_dma;
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> > index f4dda88a73f7..9ee493465c4b 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> > @@ -100,6 +100,7 @@ int dpu_rm_destroy(struct dpu_rm *rm)
> >
> >   int dpu_rm_init(struct dpu_rm *rm,
> >               const struct dpu_mdss_cfg *cat,
> > +             const struct msm_mdss_data *mdss_data,
> >               void __iomem *mmio)
> >   {
> >       int rc, i;
> > @@ -268,7 +269,7 @@ int dpu_rm_init(struct dpu_rm *rm,
> >                       continue;
> >               }
> >
> > -             hw = dpu_hw_sspp_init(sspp->id, mmio, cat);
> > +             hw = dpu_hw_sspp_init(sspp->id, mmio, cat, mdss_data);
> >               if (IS_ERR(hw)) {
> >                       rc = PTR_ERR(hw);
> >                       DPU_ERROR("failed sspp object creation: err %d\n", rc);
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> > index d62c2edb2460..2b551566cbf4 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> > @@ -40,11 +40,13 @@ struct dpu_rm {
> >    *  for all HW blocks.
> >    * @rm: DPU Resource Manager handle
> >    * @cat: Pointer to hardware catalog
> > + * @mdss_data: Pointer to MDSS / UBWC configuration
> >    * @mmio: mapped register io address of MDP
> >    * @Return: 0 on Success otherwise -ERROR
> >    */
> >   int dpu_rm_init(struct dpu_rm *rm,
> >               const struct dpu_mdss_cfg *cat,
> > +             const struct msm_mdss_data *mdss_data,
> >               void __iomem *mmio);
> >
> >   /**



-- 
With best wishes
Dmitry

Re: [PATCH 5/6] drm/msm/dpu: use MDSS data for programming SSPP

[Abhinav Kumar]

On 7/27/2023 1:39 AM, Dmitry Baryshkov wrote:
> On Thu, 27 Jul 2023 at 02:20, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 5/21/2023 10:10 AM, Dmitry Baryshkov wrote:
>>> Switch to using data from MDSS driver to program the SSPP fetch and UBWC
>>> configuration.
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 18 +++++++++++-------
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h |  7 +++++--
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     | 16 +++++++++++++++-
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h     |  1 +
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c      |  3 ++-
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h      |  2 ++
>>>    6 files changed, 36 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
>>> index cf70a9bd1034..bfd82c2921af 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
>>> @@ -8,6 +8,8 @@
>>>    #include "dpu_hw_sspp.h"
>>>    #include "dpu_kms.h"
>>>
>>> +#include "msm_mdss.h"
>>> +
>>>    #include <drm/drm_file.h>
>>>
>>>    #define DPU_FETCH_CONFIG_RESET_VALUE   0x00000087
>>> @@ -308,26 +310,26 @@ static void dpu_hw_sspp_setup_format(struct dpu_sw_pipe *pipe,
>>>                DPU_REG_WRITE(c, SSPP_FETCH_CONFIG,
>>>                        DPU_FETCH_CONFIG_RESET_VALUE |
>>>                        ctx->ubwc->highest_bank_bit << 18);
>>
>> Does this needs to be protected with if ctx->ubwc check?
> 
> Yes... And it should have been already.
> 
>>
>>> -             switch (ctx->ubwc->ubwc_version) {
>>> -             case DPU_HW_UBWC_VER_10:
>>> +             switch (ctx->ubwc->ubwc_enc_version) {
>>> +             case UBWC_1_0:
>>
>> The values of UBWC_x_x dont match the values of DPU_HW_UBWC_VER_xx.
>> What was the reason for the catalog to go with DPU_HW_UBWC_VER_xx in the
>> catalog for the encoder version in the first place? Because looking at
>> the registers UBWC_x_x is the correct value.
>>
>> If we cannot find the reason why, it should be noted in the commit text
>> that the values we are using did change.
> 
> Huh? This is just an enum. It isn't a part of uABI, nor it is written
> to the hardware.
> 

The reason is that, this switch case is moving from comparing one set of 
values to totally different ones. So atleast that should be noted.

First thing that struck me was this point because the enums UBWC_x_x and 
DPU_HW_UBWC_VER_xx dont match.

>>
>>>                        fast_clear = fmt->alpha_enable ? BIT(31) : 0;
>>>                        DPU_REG_WRITE(c, SSPP_UBWC_STATIC_CTRL,
>>>                                        fast_clear | (ctx->ubwc->ubwc_swizzle & 0x1) |
>>>                                        BIT(8) |
>>>                                        (ctx->ubwc->highest_bank_bit << 4));
>>>                        break;
>>> -             case DPU_HW_UBWC_VER_20:
>>> +             case UBWC_2_0:
>>>                        fast_clear = fmt->alpha_enable ? BIT(31) : 0;
>>>                        DPU_REG_WRITE(c, SSPP_UBWC_STATIC_CTRL,
>>>                                        fast_clear | (ctx->ubwc->ubwc_swizzle) |
>>>                                        (ctx->ubwc->highest_bank_bit << 4));
>>>                        break;
>>> -             case DPU_HW_UBWC_VER_30:
>>> +             case UBWC_3_0:
>>>                        DPU_REG_WRITE(c, SSPP_UBWC_STATIC_CTRL,
>>>                                        BIT(30) | (ctx->ubwc->ubwc_swizzle) |
>>>                                        (ctx->ubwc->highest_bank_bit << 4));
>>>                        break;
>>> -             case DPU_HW_UBWC_VER_40:
>>> +             case UBWC_4_0:
>>>                        DPU_REG_WRITE(c, SSPP_UBWC_STATIC_CTRL,
>>>                                        DPU_FORMAT_IS_YUV(fmt) ? 0 : BIT(30));
>>>                        break;
>>> @@ -793,7 +795,9 @@ static const struct dpu_sspp_cfg *_sspp_offset(enum dpu_sspp sspp,
>>>    }
>>>
>>>    struct dpu_hw_sspp *dpu_hw_sspp_init(enum dpu_sspp idx,
>>> -             void __iomem *addr, const struct dpu_mdss_cfg *catalog)
>>> +                                  void __iomem *addr,
>>> +                                  const struct dpu_mdss_cfg *catalog,
>>> +                                  const struct msm_mdss_data *mdss_data)
>>>    {
>>>        struct dpu_hw_sspp *hw_pipe;
>>>        const struct dpu_sspp_cfg *cfg;
>>> @@ -813,7 +817,7 @@ struct dpu_hw_sspp *dpu_hw_sspp_init(enum dpu_sspp idx,
>>>
>>>        /* Assign ops */
>>>        hw_pipe->catalog = catalog;
>>> -     hw_pipe->ubwc = catalog->ubwc;
>>> +     hw_pipe->ubwc = mdss_data;
>>>        hw_pipe->idx = idx;
>>>        hw_pipe->cap = cfg;
>>>        _setup_layer_ops(hw_pipe, hw_pipe->cap->features);
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
>>> index 74b98b6b3bc3..8d4ae9d24674 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
>>> @@ -351,7 +351,7 @@ struct dpu_hw_sspp {
>>>        struct dpu_hw_blk base;
>>>        struct dpu_hw_blk_reg_map hw;
>>>        const struct dpu_mdss_cfg *catalog;
>>> -     const struct dpu_ubwc_cfg *ubwc;
>>> +     const struct msm_mdss_data *ubwc;
>>>
>>>        /* Pipe */
>>>        enum dpu_sspp idx;
>>> @@ -368,9 +368,12 @@ struct dpu_kms;
>>>     * @idx:  Pipe index for which driver object is required
>>>     * @addr: Mapped register io address of MDP
>>>     * @catalog : Pointer to mdss catalog data
>>> + * @mdss_data: UBWC / MDSS configuration data
>>>     */
>>>    struct dpu_hw_sspp *dpu_hw_sspp_init(enum dpu_sspp idx,
>>> -             void __iomem *addr, const struct dpu_mdss_cfg *catalog);
>>> +                                  void __iomem *addr,
>>> +                                  const struct dpu_mdss_cfg *catalog,
>>> +                                  const struct msm_mdss_data *mdss_data);
>>>
>>>    /**
>>>     * dpu_hw_sspp_destroy(): Destroys SSPP driver context
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> index 0e7a68714e9e..a4293dc0b61b 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> @@ -22,6 +22,7 @@
>>>
>>>    #include "msm_drv.h"
>>>    #include "msm_mmu.h"
>>> +#include "msm_mdss.h"
>>>    #include "msm_gem.h"
>>>    #include "disp/msm_disp_snapshot.h"
>>>
>>> @@ -1066,7 +1067,20 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
>>>                goto power_error;
>>>        }
>>>
>>> -     rc = dpu_rm_init(&dpu_kms->rm, dpu_kms->catalog, dpu_kms->mmio);
>>> +     dpu_kms->mdss = msm_mdss_get_mdss_data(dpu_kms->pdev->dev.parent);
>>> +     if (IS_ERR(dpu_kms->mdss)) {
>>> +             rc = PTR_ERR(dpu_kms->mdss);
>>> +             DPU_ERROR("failed to get MDSS data: %d\n", rc);
>>> +             goto power_error;
>>> +     }
>>> +
>>> +     if (!dpu_kms->mdss) {
>>> +             rc = -EINVAL;
>>> +             DPU_ERROR("NULL MDSS data\n");
>>> +             goto power_error;
>>> +     }
>>> +
>>> +     rc = dpu_rm_init(&dpu_kms->rm, dpu_kms->catalog, dpu_kms->mdss, dpu_kms->mmio);
>>>        if (rc) {
>>>                DPU_ERROR("rm init failed: %d\n", rc);
>>>                goto power_error;
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
>>> index aca39a4689f4..797b4ff3e806 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
>>> @@ -69,6 +69,7 @@ struct dpu_kms {
>>>        struct msm_kms base;
>>>        struct drm_device *dev;
>>>        const struct dpu_mdss_cfg *catalog;
>>> +     const struct msm_mdss_data *mdss;
>>>
>>>        /* io/register spaces: */
>>>        void __iomem *mmio, *vbif[VBIF_MAX], *reg_dma;
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>>> index f4dda88a73f7..9ee493465c4b 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>>> @@ -100,6 +100,7 @@ int dpu_rm_destroy(struct dpu_rm *rm)
>>>
>>>    int dpu_rm_init(struct dpu_rm *rm,
>>>                const struct dpu_mdss_cfg *cat,
>>> +             const struct msm_mdss_data *mdss_data,
>>>                void __iomem *mmio)
>>>    {
>>>        int rc, i;
>>> @@ -268,7 +269,7 @@ int dpu_rm_init(struct dpu_rm *rm,
>>>                        continue;
>>>                }
>>>
>>> -             hw = dpu_hw_sspp_init(sspp->id, mmio, cat);
>>> +             hw = dpu_hw_sspp_init(sspp->id, mmio, cat, mdss_data);
>>>                if (IS_ERR(hw)) {
>>>                        rc = PTR_ERR(hw);
>>>                        DPU_ERROR("failed sspp object creation: err %d\n", rc);
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
>>> index d62c2edb2460..2b551566cbf4 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
>>> @@ -40,11 +40,13 @@ struct dpu_rm {
>>>     *  for all HW blocks.
>>>     * @rm: DPU Resource Manager handle
>>>     * @cat: Pointer to hardware catalog
>>> + * @mdss_data: Pointer to MDSS / UBWC configuration
>>>     * @mmio: mapped register io address of MDP
>>>     * @Return: 0 on Success otherwise -ERROR
>>>     */
>>>    int dpu_rm_init(struct dpu_rm *rm,
>>>                const struct dpu_mdss_cfg *cat,
>>> +             const struct msm_mdss_data *mdss_data,
>>>                void __iomem *mmio);
>>>
>>>    /**
> 
> 
> 

Re: [PATCH 5/6] drm/msm/dpu: use MDSS data for programming SSPP

[Dmitry Baryshkov]

On 27/07/2023 18:24, Abhinav Kumar wrote:
> 
> 
> On 7/27/2023 1:39 AM, Dmitry Baryshkov wrote:
>> On Thu, 27 Jul 2023 at 02:20, Abhinav Kumar 
>> <quic_abhinavk@quicinc.com> wrote:
>>>
>>>
>>>
>>> On 5/21/2023 10:10 AM, Dmitry Baryshkov wrote:
>>>> Switch to using data from MDSS driver to program the SSPP fetch and 
>>>> UBWC
>>>> configuration.
>>>>
>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>> ---
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 18 +++++++++++-------
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h |  7 +++++--
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     | 16 +++++++++++++++-
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h     |  1 +
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c      |  3 ++-
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h      |  2 ++
>>>>    6 files changed, 36 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
>>>> index cf70a9bd1034..bfd82c2921af 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
>>>> @@ -8,6 +8,8 @@
>>>>    #include "dpu_hw_sspp.h"
>>>>    #include "dpu_kms.h"
>>>>
>>>> +#include "msm_mdss.h"
>>>> +
>>>>    #include <drm/drm_file.h>
>>>>
>>>>    #define DPU_FETCH_CONFIG_RESET_VALUE   0x00000087
>>>> @@ -308,26 +310,26 @@ static void dpu_hw_sspp_setup_format(struct 
>>>> dpu_sw_pipe *pipe,
>>>>                DPU_REG_WRITE(c, SSPP_FETCH_CONFIG,
>>>>                        DPU_FETCH_CONFIG_RESET_VALUE |
>>>>                        ctx->ubwc->highest_bank_bit << 18);
>>>
>>> Does this needs to be protected with if ctx->ubwc check?
>>
>> Yes... And it should have been already.
>>
>>>
>>>> -             switch (ctx->ubwc->ubwc_version) {
>>>> -             case DPU_HW_UBWC_VER_10:
>>>> +             switch (ctx->ubwc->ubwc_enc_version) {
>>>> +             case UBWC_1_0:
>>>
>>> The values of UBWC_x_x dont match the values of DPU_HW_UBWC_VER_xx.
>>> What was the reason for the catalog to go with DPU_HW_UBWC_VER_xx in the
>>> catalog for the encoder version in the first place? Because looking at
>>> the registers UBWC_x_x is the correct value.
>>>
>>> If we cannot find the reason why, it should be noted in the commit text
>>> that the values we are using did change.
>>
>> Huh? This is just an enum. It isn't a part of uABI, nor it is written
>> to the hardware.
>>
> 
> The reason is that, this switch case is moving from comparing one set of 
> values to totally different ones. So atleast that should be noted.
> 
> First thing that struck me was this point because the enums UBWC_x_x and 
> DPU_HW_UBWC_VER_xx dont match.
> 

Do you have any proposed text in mind?

-- 
With best wishes
Dmitry

Re: [PATCH 5/6] drm/msm/dpu: use MDSS data for programming SSPP

[Abhinav Kumar]

On 7/27/2023 8:26 AM, Dmitry Baryshkov wrote:
> On 27/07/2023 18:24, Abhinav Kumar wrote:
>>
>>
>> On 7/27/2023 1:39 AM, Dmitry Baryshkov wrote:
>>> On Thu, 27 Jul 2023 at 02:20, Abhinav Kumar 
>>> <quic_abhinavk@quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 5/21/2023 10:10 AM, Dmitry Baryshkov wrote:
>>>>> Switch to using data from MDSS driver to program the SSPP fetch and 
>>>>> UBWC
>>>>> configuration.
>>>>>
>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>> ---
>>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 18 +++++++++++-------
>>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h |  7 +++++--
>>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     | 16 +++++++++++++++-
>>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h     |  1 +
>>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c      |  3 ++-
>>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h      |  2 ++
>>>>>    6 files changed, 36 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c 
>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
>>>>> index cf70a9bd1034..bfd82c2921af 100644
>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
>>>>> @@ -8,6 +8,8 @@
>>>>>    #include "dpu_hw_sspp.h"
>>>>>    #include "dpu_kms.h"
>>>>>
>>>>> +#include "msm_mdss.h"
>>>>> +
>>>>>    #include <drm/drm_file.h>
>>>>>
>>>>>    #define DPU_FETCH_CONFIG_RESET_VALUE   0x00000087
>>>>> @@ -308,26 +310,26 @@ static void dpu_hw_sspp_setup_format(struct 
>>>>> dpu_sw_pipe *pipe,
>>>>>                DPU_REG_WRITE(c, SSPP_FETCH_CONFIG,
>>>>>                        DPU_FETCH_CONFIG_RESET_VALUE |
>>>>>                        ctx->ubwc->highest_bank_bit << 18);
>>>>
>>>> Does this needs to be protected with if ctx->ubwc check?
>>>
>>> Yes... And it should have been already.
>>>
>>>>
>>>>> -             switch (ctx->ubwc->ubwc_version) {
>>>>> -             case DPU_HW_UBWC_VER_10:
>>>>> +             switch (ctx->ubwc->ubwc_enc_version) {
>>>>> +             case UBWC_1_0:
>>>>
>>>> The values of UBWC_x_x dont match the values of DPU_HW_UBWC_VER_xx.
>>>> What was the reason for the catalog to go with DPU_HW_UBWC_VER_xx in 
>>>> the
>>>> catalog for the encoder version in the first place? Because looking at
>>>> the registers UBWC_x_x is the correct value.
>>>>
>>>> If we cannot find the reason why, it should be noted in the commit text
>>>> that the values we are using did change.
>>>
>>> Huh? This is just an enum. It isn't a part of uABI, nor it is written
>>> to the hardware.
>>>
>>
>> The reason is that, this switch case is moving from comparing one set 
>> of values to totally different ones. So atleast that should be noted.
>>
>> First thing that struck me was this point because the enums UBWC_x_x 
>> and DPU_HW_UBWC_VER_xx dont match.
>>
> 
> Do you have any proposed text in mind?
> 

I was doing some checking about this. The issue was that when this enum 
was made, it missed using the SDE_HW_UBWC_VER macro


75 enum {
76 	DPU_HW_UBWC_VER_10 = 0x100,
77 	DPU_HW_UBWC_VER_20 = 0x200,
78 	DPU_HW_UBWC_VER_30 = 0x300,
79 	DPU_HW_UBWC_VER_40 = 0x400,
80 };
81

so something like this:

183  */
184 enum {
185 	SDE_HW_UBWC_VER_10 = SDE_HW_UBWC_VER(0x100),
186 	SDE_HW_UBWC_VER_20 = SDE_HW_UBWC_VER(0x200),
187 	SDE_HW_UBWC_VER_30 = SDE_HW_UBWC_VER(0x300),
188 	SDE_HW_UBWC_VER_40 = SDE_HW_UBWC_VER(0x400),
189 	SDE_HW_UBWC_VER_43 = SDE_HW_UBWC_VER(0x431),
190 };

This macro handles that conversion under the hood.

So I would write something like this

"This also corrects the usage of UBWC version which was incorrect from 
the beginning because of the enum storing the DPU_HW_UBWC_*** missed out 
the conversion to the full UBWC version"

Re: [PATCH 5/6] drm/msm/dpu: use MDSS data for programming SSPP

[Dmitry Baryshkov]

On Fri, 28 Jul 2023 at 22:25, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 7/27/2023 8:26 AM, Dmitry Baryshkov wrote:
> > On 27/07/2023 18:24, Abhinav Kumar wrote:
> >>
> >>
> >> On 7/27/2023 1:39 AM, Dmitry Baryshkov wrote:
> >>> On Thu, 27 Jul 2023 at 02:20, Abhinav Kumar
> >>> <quic_abhinavk@quicinc.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 5/21/2023 10:10 AM, Dmitry Baryshkov wrote:
> >>>>> Switch to using data from MDSS driver to program the SSPP fetch and
> >>>>> UBWC
> >>>>> configuration.
> >>>>>
> >>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >>>>> ---
> >>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 18 +++++++++++-------
> >>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h |  7 +++++--
> >>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     | 16 +++++++++++++++-
> >>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h     |  1 +
> >>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c      |  3 ++-
> >>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h      |  2 ++
> >>>>>    6 files changed, 36 insertions(+), 11 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
> >>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
> >>>>> index cf70a9bd1034..bfd82c2921af 100644
> >>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
> >>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
> >>>>> @@ -8,6 +8,8 @@
> >>>>>    #include "dpu_hw_sspp.h"
> >>>>>    #include "dpu_kms.h"
> >>>>>
> >>>>> +#include "msm_mdss.h"
> >>>>> +
> >>>>>    #include <drm/drm_file.h>
> >>>>>
> >>>>>    #define DPU_FETCH_CONFIG_RESET_VALUE   0x00000087
> >>>>> @@ -308,26 +310,26 @@ static void dpu_hw_sspp_setup_format(struct
> >>>>> dpu_sw_pipe *pipe,
> >>>>>                DPU_REG_WRITE(c, SSPP_FETCH_CONFIG,
> >>>>>                        DPU_FETCH_CONFIG_RESET_VALUE |
> >>>>>                        ctx->ubwc->highest_bank_bit << 18);
> >>>>
> >>>> Does this needs to be protected with if ctx->ubwc check?
> >>>
> >>> Yes... And it should have been already.
> >>>
> >>>>
> >>>>> -             switch (ctx->ubwc->ubwc_version) {
> >>>>> -             case DPU_HW_UBWC_VER_10:
> >>>>> +             switch (ctx->ubwc->ubwc_enc_version) {
> >>>>> +             case UBWC_1_0:
> >>>>
> >>>> The values of UBWC_x_x dont match the values of DPU_HW_UBWC_VER_xx.
> >>>> What was the reason for the catalog to go with DPU_HW_UBWC_VER_xx in
> >>>> the
> >>>> catalog for the encoder version in the first place? Because looking at
> >>>> the registers UBWC_x_x is the correct value.
> >>>>
> >>>> If we cannot find the reason why, it should be noted in the commit text
> >>>> that the values we are using did change.
> >>>
> >>> Huh? This is just an enum. It isn't a part of uABI, nor it is written
> >>> to the hardware.
> >>>
> >>
> >> The reason is that, this switch case is moving from comparing one set
> >> of values to totally different ones. So atleast that should be noted.
> >>
> >> First thing that struck me was this point because the enums UBWC_x_x
> >> and DPU_HW_UBWC_VER_xx dont match.
> >>
> >
> > Do you have any proposed text in mind?
> >
>
> I was doing some checking about this. The issue was that when this enum
> was made, it missed using the SDE_HW_UBWC_VER macro
>
>
> 75 enum {
> 76      DPU_HW_UBWC_VER_10 = 0x100,
> 77      DPU_HW_UBWC_VER_20 = 0x200,
> 78      DPU_HW_UBWC_VER_30 = 0x300,
> 79      DPU_HW_UBWC_VER_40 = 0x400,
> 80 };
> 81
>
> so something like this:
>
> 183  */
> 184 enum {
> 185     SDE_HW_UBWC_VER_10 = SDE_HW_UBWC_VER(0x100),
> 186     SDE_HW_UBWC_VER_20 = SDE_HW_UBWC_VER(0x200),
> 187     SDE_HW_UBWC_VER_30 = SDE_HW_UBWC_VER(0x300),
> 188     SDE_HW_UBWC_VER_40 = SDE_HW_UBWC_VER(0x400),
> 189     SDE_HW_UBWC_VER_43 = SDE_HW_UBWC_VER(0x431),
> 190 };
>
> This macro handles that conversion under the hood.
>
> So I would write something like this
>
> "This also corrects the usage of UBWC version which was incorrect from
> the beginning because of the enum storing the DPU_HW_UBWC_*** missed out
> the conversion to the full UBWC version"

I don't like the word 'correcting', as it makes one think there was an
issue beforehand. There were no real issues. So I'll try to cook
something up for the next iteration.



-- 
With best wishes
Dmitry