[PATCH v3 0/4] drm/msm: provide migration path from MDP5 to DPU driver

[PATCH v3 0/4] drm/msm: provide migration path from MDP5 to DPU driver

[Dmitry Baryshkov]

Over the last several years the DPU driver has been actively developed,
while the MDP5 is mostly in the maintenance mode. This results in some
features being available only in the DPU driver. For example, bandwidth
scaling, writeback support, properly supported bonded DSI aka dual DSI
support, DSC (display stream compression).

All the pre-SDM845 platforms were originally supported by the MDP5
driver only. However it is possible and easy to support some of the
older SoCs in the DPU driver. For example in the v5.18 it got
support for MSM8998.  This can not be considered as a proper migration,
since there msm8998.dtsi didn't describe the display hardware
beforehand. Instead new bindings were added, making MSM8998 just another
display hardware to be supported by the DPU driver.

This series provides a way to gradually migrate support for several
existing and well-supported SoCs from the MDP5 to the DPU driver without
changing the DT. From the user experience point of view this is
facilitated by the `msm.prefer_mdp5' kernel param. If the parameter is
set to `true' (current default), all `shared' platforms will be handled
by the MDP5 driver. If the switch is flipped to `false' (or if the MDP5
driver is disabled), these platforms will be handled by the DPU driver.
Handling this by the modparam (rather than solely by kernel config)
allows one to easly switch between the drivers, simplifying testing.

This series implements support for two DPU 3.n platforms, SDM660 and
SDM630. The MSM8996 support was a part of the previous iterations of
this patchset, but it was removed in v3. It requires additional
development and testing.

In theory after additional testing we can drop most of migration code
and some parts of MDP5 driver. The proposed boundary is to move all
platforms supporting cursor planes to the DPU driver, while limiting
MDP5 to support only the older platforms which implement cursor as a
part of the LM hardware block (MSM8974, APQ8084, MSM8x26, MSM8x16 and
MSM8x39).

Changes since v2:
- Rebased on top of linux-next
- After additional consideration dropped MSM8996 patch. It will be
  reiterated later, once the generic migration framework is accepted
  (and after we implement scalers support for that platform).

Changes since v1:
- Dropped accepted patches
- Rebased on top of updated [1]
- Added defines for MDSS hw revisions (Stephen)
- Changed msm_mdss_generate_mdp5_mdss_data() to return const struct
  pointer (Stephen)
- Fixed error handling in msm_ioremap_mdss() (Stephen)

[1] https://patchwork.freedesktop.org/series/119804/

---
Dmitry Baryshkov (4):
      drm/msm/mdss: generate MDSS data for MDP5 platforms
      drm/msm/dpu: support binding to the mdp5 devices
      drm/msm: add a kernel param to select between MDP5 and DPU drivers
      drm/msm/dpu: add support for SDM660 and SDM630 platforms

 .../gpu/drm/msm/disp/dpu1/catalog/dpu_3_2_sdm660.h | 291 +++++++++++++++++++++
 .../gpu/drm/msm/disp/dpu1/catalog/dpu_3_3_sdm630.h | 225 ++++++++++++++++
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c     |   2 +
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h     |   2 +
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c            | 103 ++++++--
 drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c           |   3 +
 drivers/gpu/drm/msm/msm_drv.c                      |  33 +++
 drivers/gpu/drm/msm/msm_drv.h                      |   4 +
 drivers/gpu/drm/msm/msm_io_utils.c                 |  13 +
 drivers/gpu/drm/msm/msm_mdss.c                     |  51 ++++
 10 files changed, 706 insertions(+), 21 deletions(-)
---
base-commit: 39676dfe52331dba909c617f213fdb21015c8d10
change-id: 20240105-fd-migrate-mdp5-6a2aa51bc83b

Best regards,
-- 
Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

[PATCH v3 1/4] drm/msm/mdss: generate MDSS data for MDP5 platforms

[Dmitry Baryshkov]

Older (mdp5) platforms do not use per-SoC compatible strings. Instead
they use a single compat entry 'qcom,mdss'. To facilitate migrating
these platforms to the DPU driver provide a way to generate the MDSS /
UBWC data at runtime, when the DPU driver asks for it.

It is not possible to generate this data structure at the probe time,
since some platforms might not have MDP_CLK enabled, which makes reading
HW_REV register return 0.

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

diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
index 455b2e3a0cdd..566a5dd5b8e8 100644
--- a/drivers/gpu/drm/msm/msm_mdss.c
+++ b/drivers/gpu/drm/msm/msm_mdss.c
@@ -3,6 +3,7 @@
  * Copyright (c) 2018, The Linux Foundation
  */
 
+#include <linux/bitfield.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/interconnect.h>
@@ -213,6 +214,49 @@ static void msm_mdss_setup_ubwc_dec_40(struct msm_mdss *msm_mdss)
 	}
 }
 
+#define MDSS_HW_MAJ_MIN		GENMASK(31, 16)
+
+#define MDSS_HW_MSM8996		0x1007
+#define MDSS_HW_MSM8937		0x100e
+#define MDSS_HW_MSM8956		0x1010
+#define MDSS_HW_MSM8998		0x3000
+#define MDSS_HW_SDM660		0x3002
+#define MDSS_HW_SDM630		0x3003
+
+/*
+ * MDP5 platforms use generic qcom,mdp5 compat string, so we have to generate this data
+ */
+static const struct msm_mdss_data *msm_mdss_generate_mdp5_mdss_data(struct msm_mdss *mdss)
+{
+	struct msm_mdss_data *data;
+	u32 hw_rev;
+
+	data = devm_kzalloc(mdss->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return NULL;
+
+	hw_rev = readl_relaxed(mdss->mmio + HW_REV);
+	hw_rev = FIELD_GET(MDSS_HW_MAJ_MIN, hw_rev);
+
+	if (hw_rev == MDSS_HW_MSM8996 ||
+	    hw_rev == MDSS_HW_MSM8937 ||
+	    hw_rev == MDSS_HW_MSM8956 ||
+	    hw_rev == MDSS_HW_MSM8998 ||
+	    hw_rev == MDSS_HW_SDM660 ||
+	    hw_rev == MDSS_HW_SDM630) {
+		data->ubwc_dec_version = UBWC_1_0;
+		data->ubwc_enc_version = UBWC_1_0;
+	}
+
+	if (hw_rev == MDSS_HW_MSM8996 ||
+	    hw_rev == MDSS_HW_MSM8998)
+		data->highest_bank_bit = 2;
+	else
+		data->highest_bank_bit = 1;
+
+	return data;
+}
+
 const struct msm_mdss_data *msm_mdss_get_mdss_data(struct device *dev)
 {
 	struct msm_mdss *mdss;
@@ -222,6 +266,13 @@ const struct msm_mdss_data *msm_mdss_get_mdss_data(struct device *dev)
 
 	mdss = dev_get_drvdata(dev);
 
+	/*
+	 * We could not do it at the probe time, since hw revision register was
+	 * not readable. Fill data structure now for the MDP5 platforms.
+	 */
+	if (!mdss->mdss_data && mdss->is_mdp5)
+		mdss->mdss_data = msm_mdss_generate_mdp5_mdss_data(mdss);
+
 	return mdss->mdss_data;
 }
 

-- 
2.39.2

[PATCH v3 2/4] drm/msm/dpu: support binding to the mdp5 devices

[Dmitry Baryshkov]

Existing MDP5 devices have slightly different bindings. The main
register region is called `mdp_phys' instead of `mdp'. Also vbif
register regions are a part of the parent, MDSS device. Add support for
handling this binding differences.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 98 ++++++++++++++++++++++++++-------
 drivers/gpu/drm/msm/msm_drv.h           |  3 +
 drivers/gpu/drm/msm/msm_io_utils.c      | 13 +++++
 3 files changed, 93 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 723cc1d82143..aa9e0ad33ebb 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -1197,6 +1197,78 @@ static int dpu_kms_init(struct drm_device *ddev)
 	return 0;
 }
 
+static int dpu_kms_mmap_mdp5(struct dpu_kms *dpu_kms)
+{
+	struct platform_device *pdev = dpu_kms->pdev;
+	struct platform_device *mdss_dev;
+	int ret;
+
+	if (dpu_kms->pdev->dev.bus != &platform_bus_type)
+		return -EINVAL;
+
+	mdss_dev = to_platform_device(dpu_kms->pdev->dev.parent);
+
+	dpu_kms->mmio = msm_ioremap(pdev, "mdp_phys");
+	if (IS_ERR(dpu_kms->mmio)) {
+		ret = PTR_ERR(dpu_kms->mmio);
+		DPU_ERROR("mdp register memory map failed: %d\n", ret);
+		dpu_kms->mmio = NULL;
+		return ret;
+	}
+	DRM_DEBUG("mapped dpu address space @%pK\n", dpu_kms->mmio);
+
+	dpu_kms->vbif[VBIF_RT] = msm_ioremap_mdss(mdss_dev,
+						  dpu_kms->pdev,
+						  "vbif_phys");
+	if (IS_ERR(dpu_kms->vbif[VBIF_RT])) {
+		ret = PTR_ERR(dpu_kms->vbif[VBIF_RT]);
+		DPU_ERROR("vbif register memory map failed: %d\n", ret);
+		dpu_kms->vbif[VBIF_RT] = NULL;
+		return ret;
+	}
+
+	dpu_kms->vbif[VBIF_NRT] = msm_ioremap_mdss(mdss_dev,
+						   dpu_kms->pdev,
+						   "vbif_nrt_phys");
+	if (IS_ERR(dpu_kms->vbif[VBIF_NRT])) {
+		dpu_kms->vbif[VBIF_NRT] = NULL;
+		DPU_DEBUG("VBIF NRT is not defined");
+	}
+
+	return 0;
+}
+
+static int dpu_kms_mmap_dpu(struct dpu_kms *dpu_kms)
+{
+	struct platform_device *pdev = dpu_kms->pdev;
+	int ret;
+
+	dpu_kms->mmio = msm_ioremap(pdev, "mdp");
+	if (IS_ERR(dpu_kms->mmio)) {
+		ret = PTR_ERR(dpu_kms->mmio);
+		DPU_ERROR("mdp register memory map failed: %d\n", ret);
+		dpu_kms->mmio = NULL;
+		return ret;
+	}
+	DRM_DEBUG("mapped dpu address space @%pK\n", dpu_kms->mmio);
+
+	dpu_kms->vbif[VBIF_RT] = msm_ioremap(pdev, "vbif");
+	if (IS_ERR(dpu_kms->vbif[VBIF_RT])) {
+		ret = PTR_ERR(dpu_kms->vbif[VBIF_RT]);
+		DPU_ERROR("vbif register memory map failed: %d\n", ret);
+		dpu_kms->vbif[VBIF_RT] = NULL;
+		return ret;
+	}
+
+	dpu_kms->vbif[VBIF_NRT] = msm_ioremap_quiet(pdev, "vbif_nrt");
+	if (IS_ERR(dpu_kms->vbif[VBIF_NRT])) {
+		dpu_kms->vbif[VBIF_NRT] = NULL;
+		DPU_DEBUG("VBIF NRT is not defined");
+	}
+
+	return 0;
+}
+
 static int dpu_dev_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -1230,28 +1302,12 @@ static int dpu_dev_probe(struct platform_device *pdev)
 
 	dpu_kms->base.irq = irq;
 
-	dpu_kms->mmio = msm_ioremap(pdev, "mdp");
-	if (IS_ERR(dpu_kms->mmio)) {
-		ret = PTR_ERR(dpu_kms->mmio);
-		DPU_ERROR("mdp register memory map failed: %d\n", ret);
-		dpu_kms->mmio = NULL;
-		return ret;
-	}
-	DRM_DEBUG("mapped dpu address space @%pK\n", dpu_kms->mmio);
-
-	dpu_kms->vbif[VBIF_RT] = msm_ioremap(pdev, "vbif");
-	if (IS_ERR(dpu_kms->vbif[VBIF_RT])) {
-		ret = PTR_ERR(dpu_kms->vbif[VBIF_RT]);
-		DPU_ERROR("vbif register memory map failed: %d\n", ret);
-		dpu_kms->vbif[VBIF_RT] = NULL;
+	if (of_device_is_compatible(dpu_kms->pdev->dev.of_node, "qcom,mdp5"))
+		ret = dpu_kms_mmap_mdp5(dpu_kms);
+	else
+		ret = dpu_kms_mmap_dpu(dpu_kms);
+	if (ret)
 		return ret;
-	}
-
-	dpu_kms->vbif[VBIF_NRT] = msm_ioremap_quiet(pdev, "vbif_nrt");
-	if (IS_ERR(dpu_kms->vbif[VBIF_NRT])) {
-		dpu_kms->vbif[VBIF_NRT] = NULL;
-		DPU_DEBUG("VBIF NRT is not defined");
-	}
 
 	ret = dpu_kms_parse_data_bus_icc_path(dpu_kms);
 	if (ret)
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 16a7cbc0b7dd..01e783130054 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -476,6 +476,9 @@ void __iomem *msm_ioremap(struct platform_device *pdev, const char *name);
 void __iomem *msm_ioremap_size(struct platform_device *pdev, const char *name,
 		phys_addr_t *size);
 void __iomem *msm_ioremap_quiet(struct platform_device *pdev, const char *name);
+void __iomem *msm_ioremap_mdss(struct platform_device *mdss_pdev,
+			       struct platform_device *dev,
+			       const char *name);
 
 struct icc_path *msm_icc_get(struct device *dev, const char *name);
 
diff --git a/drivers/gpu/drm/msm/msm_io_utils.c b/drivers/gpu/drm/msm/msm_io_utils.c
index 59d2788c4510..afedd61c3e28 100644
--- a/drivers/gpu/drm/msm/msm_io_utils.c
+++ b/drivers/gpu/drm/msm/msm_io_utils.c
@@ -50,6 +50,19 @@ struct clk *msm_clk_get(struct platform_device *pdev, const char *name)
 	return clk;
 }
 
+void __iomem *msm_ioremap_mdss(struct platform_device *mdss_pdev,
+			       struct platform_device *pdev,
+			       const char *name)
+{
+	struct resource *res;
+
+	res = platform_get_resource_byname(mdss_pdev, IORESOURCE_MEM, name);
+	if (!res)
+		return ERR_PTR(-EINVAL);
+
+	return devm_ioremap_resource(&pdev->dev, res);
+}
+
 static void __iomem *_msm_ioremap(struct platform_device *pdev, const char *name,
 				  bool quiet, phys_addr_t *psize)
 {

-- 
2.39.2

[PATCH v3 3/4] drm/msm: add a kernel param to select between MDP5 and DPU drivers

[Dmitry Baryshkov]

For some of the platforms (e.g. SDM660, SDM630, MSM8996, etc.) it is
possible to support this platform via the DPU driver (e.g. to provide
support for DP, multirect, etc). Add a modparam to be able to switch
between these two drivers.

All platforms supported by both drivers are by default handled by the
MDP5 driver. To let them be handled by the DPU driver pass the
`msm.prefer_mdp5=false` kernel param.

Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  |  3 +++
 drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c |  3 +++
 drivers/gpu/drm/msm/msm_drv.c            | 31 +++++++++++++++++++++++++++++++
 drivers/gpu/drm/msm/msm_drv.h            |  1 +
 4 files changed, 38 insertions(+)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index aa9e0ad33ebb..8f11a98491a1 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -1276,6 +1276,9 @@ static int dpu_dev_probe(struct platform_device *pdev)
 	int irq;
 	int ret = 0;
 
+	if (!msm_disp_drv_should_bind(&pdev->dev, true))
+		return -ENODEV;
+
 	dpu_kms = devm_kzalloc(dev, sizeof(*dpu_kms), GFP_KERNEL);
 	if (!dpu_kms)
 		return -ENOMEM;
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
index 0827634664ae..43d05851c54d 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
@@ -866,6 +866,9 @@ static int mdp5_dev_probe(struct platform_device *pdev)
 
 	DBG("");
 
+	if (!msm_disp_drv_should_bind(&pdev->dev, false))
+		return -ENODEV;
+
 	mdp5_kms = devm_kzalloc(&pdev->dev, sizeof(*mdp5_kms), GFP_KERNEL);
 	if (!mdp5_kms)
 		return -ENOMEM;
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 50b65ffc24b1..ef57586fbeca 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -969,6 +969,37 @@ static int add_components_mdp(struct device *master_dev,
 	return 0;
 }
 
+#if !IS_REACHABLE(CONFIG_DRM_MSM_MDP5) || !IS_REACHABLE(CONFIG_DRM_MSM_DPU)
+bool msm_disp_drv_should_bind(struct device *dev, bool mdp5_driver)
+{
+	/* If just a single driver is enabled, use it no matter what */
+	return true;
+}
+#else
+
+static bool prefer_mdp5 = true;
+MODULE_PARM_DESC(prefer_mdp5, "Select whether MDP5 or DPU driver should be preferred");
+module_param(prefer_mdp5, bool, 0444);
+
+/* list all platforms supported by both mdp5 and dpu drivers */
+static const char *const msm_mdp5_dpu_migration[] = {
+	NULL,
+};
+
+bool msm_disp_drv_should_bind(struct device *dev, bool dpu_driver)
+{
+	/* If it is not an MDP5 device, do not try MDP5 driver */
+	if (!of_device_is_compatible(dev->of_node, "qcom,mdp5"))
+		return dpu_driver;
+
+	/* If it is not in the migration list, use MDP5 */
+	if (!of_device_compatible_match(dev->of_node, msm_mdp5_dpu_migration))
+		return !dpu_driver;
+
+	return prefer_mdp5 ? !dpu_driver : dpu_driver;
+}
+#endif
+
 /*
  * We don't know what's the best binding to link the gpu with the drm device.
  * Fow now, we just hunt for all the possible gpus that we support, and add them
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 01e783130054..762e13e2df74 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -563,5 +563,6 @@ int msm_drv_probe(struct device *dev,
 	struct msm_kms *kms);
 void msm_kms_shutdown(struct platform_device *pdev);
 
+bool msm_disp_drv_should_bind(struct device *dev, bool dpu_driver);
 
 #endif /* __MSM_DRV_H__ */

-- 
2.39.2

[PATCH v3 4/4] drm/msm/dpu: add support for SDM660 and SDM630 platforms

[Dmitry Baryshkov]

Bring in hardware support for the SDM660 and SDM630 platforms, which
belong to the same DPU generation as MSM8998.

Note, by default these platforms are still handled by the MDP5 driver
unless the `msm.prefer_mdp5=false' parameter is provided.

Co-developed-by: Konrad Dybcio <konrad.dybcio@somainline.org>
Signed-off-by: Konrad Dybcio <konrad.dybcio@somainline.org>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 .../gpu/drm/msm/disp/dpu1/catalog/dpu_3_2_sdm660.h | 291 +++++++++++++++++++++
 .../gpu/drm/msm/disp/dpu1/catalog/dpu_3_3_sdm630.h | 225 ++++++++++++++++
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c     |   2 +
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h     |   2 +
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c            |   2 +
 drivers/gpu/drm/msm/msm_drv.c                      |   2 +
 6 files changed, 524 insertions(+)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_2_sdm660.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_2_sdm660.h
new file mode 100644
index 000000000000..424815e7fb7d
--- /dev/null
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_2_sdm660.h
@@ -0,0 +1,291 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2023. Linaro Inc. All rights reserved.
+ */
+
+#ifndef _DPU_3_2_SDM660_H
+#define _DPU_3_2_SDM660_H
+
+static const struct dpu_caps sdm660_dpu_caps = {
+	.max_mixer_width = DEFAULT_DPU_OUTPUT_LINE_WIDTH,
+	.max_mixer_blendstages = 0x7,
+	.has_src_split = true,
+	.has_dim_layer = true,
+	.has_idle_pc = true,
+	.has_3d_merge = true,
+	.max_linewidth = DEFAULT_DPU_OUTPUT_LINE_WIDTH,
+	.pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
+	.max_hdeci_exp = MAX_HORZ_DECIMATION,
+	.max_vdeci_exp = MAX_VERT_DECIMATION,
+};
+
+static const struct dpu_mdp_cfg sdm660_mdp = {
+	.name = "top_0",
+	.base = 0x0, .len = 0x458,
+	.features = BIT(DPU_MDP_VSYNC_SEL),
+	.clk_ctrls = {
+		[DPU_CLK_CTRL_VIG0] = { .reg_off = 0x2ac, .bit_off = 0 },
+		[DPU_CLK_CTRL_VIG1] = { .reg_off = 0x2b4, .bit_off = 0 },
+		[DPU_CLK_CTRL_DMA0] = { .reg_off = 0x2ac, .bit_off = 8 },
+		[DPU_CLK_CTRL_DMA1] = { .reg_off = 0x2b4, .bit_off = 8 },
+		[DPU_CLK_CTRL_DMA2] = { .reg_off = 0x2c4, .bit_off = 8 },
+		[DPU_CLK_CTRL_CURSOR0] = { .reg_off = 0x3a8, .bit_off = 16 },
+	},
+};
+
+static const struct dpu_ctl_cfg sdm660_ctl[] = {
+	{
+		.name = "ctl_0", .id = CTL_0,
+		.base = 0x1000, .len = 0x94,
+		.features = BIT(DPU_CTL_SPLIT_DISPLAY),
+		.intr_start = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR2, 9),
+	}, {
+		.name = "ctl_1", .id = CTL_1,
+		.base = 0x1200, .len = 0x94,
+		.intr_start = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR2, 10),
+	}, {
+		.name = "ctl_2", .id = CTL_2,
+		.base = 0x1400, .len = 0x94,
+		.features = BIT(DPU_CTL_SPLIT_DISPLAY),
+		.intr_start = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR2, 11),
+	}, {
+		.name = "ctl_3", .id = CTL_3,
+		.base = 0x1600, .len = 0x94,
+		.intr_start = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR2, 12),
+	}, {
+		.name = "ctl_4", .id = CTL_4,
+		.base = 0x1800, .len = 0x94,
+		.intr_start = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR2, 13),
+	},
+};
+
+static const struct dpu_sspp_cfg sdm660_sspp[] = {
+	{
+		.name = "sspp_0", .id = SSPP_VIG0,
+		.base = 0x4000, .len = 0x1ac,
+		.features = VIG_MSM8998_MASK,
+		.sblk = &dpu_vig_sblk_qseed3_1_2,
+		.xin_id = 0,
+		.type = SSPP_TYPE_VIG,
+		.clk_ctrl = DPU_CLK_CTRL_VIG0,
+	}, {
+		.name = "sspp_1", .id = SSPP_VIG1,
+		.base = 0x6000, .len = 0x1ac,
+		.features = VIG_MSM8998_MASK,
+		.sblk = &dpu_vig_sblk_qseed3_1_2,
+		.xin_id = 4,
+		.type = SSPP_TYPE_VIG,
+		.clk_ctrl = DPU_CLK_CTRL_VIG1,
+	}, {
+		.name = "sspp_8", .id = SSPP_DMA0,
+		.base = 0x24000, .len = 0x1ac,
+		.features = DMA_MSM8998_MASK,
+		.sblk = &dpu_dma_sblk,
+		.xin_id = 1,
+		.type = SSPP_TYPE_DMA,
+		.clk_ctrl = DPU_CLK_CTRL_DMA0,
+	}, {
+		.name = "sspp_9", .id = SSPP_DMA1,
+		.base = 0x26000, .len = 0x1ac,
+		.features = DMA_MSM8998_MASK,
+		.sblk = &dpu_dma_sblk,
+		.xin_id = 5,
+		.type = SSPP_TYPE_DMA,
+		.clk_ctrl = DPU_CLK_CTRL_DMA1,
+	}, {
+		.name = "sspp_10", .id = SSPP_DMA2,
+		.base = 0x28000, .len = 0x1ac,
+		.features = DMA_CURSOR_MSM8998_MASK,
+		.sblk = &dpu_dma_sblk,
+		.xin_id = 9,
+		.type = SSPP_TYPE_DMA,
+		.clk_ctrl = DPU_CLK_CTRL_DMA2,
+	},
+};
+
+static const struct dpu_lm_cfg sdm660_lm[] = {
+	{
+		.name = "lm_0", .id = LM_0,
+		.base = 0x44000, .len = 0x320,
+		.features = MIXER_MSM8998_MASK,
+		.sblk = &msm8998_lm_sblk,
+		.lm_pair = LM_1,
+		.pingpong = PINGPONG_0,
+		.dspp = DSPP_0,
+	}, {
+		.name = "lm_1", .id = LM_1,
+		.base = 0x45000, .len = 0x320,
+		.features = MIXER_MSM8998_MASK,
+		.sblk = &msm8998_lm_sblk,
+		.lm_pair = LM_0,
+		.pingpong = PINGPONG_1,
+		.dspp = DSPP_1,
+	}, {
+		.name = "lm_2", .id = LM_2,
+		.base = 0x46000, .len = 0x320,
+		.features = MIXER_MSM8998_MASK,
+		.sblk = &msm8998_lm_sblk,
+		.lm_pair = LM_5,
+		.pingpong = PINGPONG_2,
+	}, {
+		.name = "lm_5", .id = LM_5,
+		.base = 0x49000, .len = 0x320,
+		.features = MIXER_MSM8998_MASK,
+		.sblk = &msm8998_lm_sblk,
+		.lm_pair = LM_2,
+		.pingpong = PINGPONG_3,
+	},
+};
+
+static const struct dpu_pingpong_cfg sdm660_pp[] = {
+	{
+		.name = "pingpong_0", .id = PINGPONG_0,
+		.base = 0x70000, .len = 0xd4,
+		.features = PINGPONG_SDM845_TE2_MASK,
+		.sblk = &sdm845_pp_sblk_te,
+		.intr_done = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 8),
+		.intr_rdptr = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 12),
+	}, {
+		.name = "pingpong_1", .id = PINGPONG_1,
+		.base = 0x70800, .len = 0xd4,
+		.features = PINGPONG_SDM845_TE2_MASK,
+		.sblk = &sdm845_pp_sblk_te,
+		.intr_done = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 9),
+		.intr_rdptr = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 13),
+	}, {
+		.name = "pingpong_2", .id = PINGPONG_2,
+		.base = 0x71000, .len = 0xd4,
+		.features = PINGPONG_SDM845_MASK,
+		.sblk = &sdm845_pp_sblk,
+		.intr_done = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 10),
+		.intr_rdptr = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 14),
+	}, {
+		.name = "pingpong_3", .id = PINGPONG_3,
+		.base = 0x71800, .len = 0xd4,
+		.features = PINGPONG_SDM845_MASK,
+		.sblk = &sdm845_pp_sblk,
+		.intr_done = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 11),
+		.intr_rdptr = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 15),
+	},
+};
+
+static const struct dpu_dsc_cfg sdm660_dsc[] = {
+	{
+		.name = "dsc_0", .id = DSC_0,
+		.base = 0x80000, .len = 0x140,
+	}, {
+		.name = "dsc_1", .id = DSC_1,
+		.base = 0x80400, .len = 0x140,
+	},
+};
+
+static const struct dpu_dspp_cfg sdm660_dspp[] = {
+	{
+		.name = "dspp_0", .id = DSPP_0,
+		.base = 0x54000, .len = 0x1800,
+		.features = DSPP_SC7180_MASK,
+		.sblk = &msm8998_dspp_sblk,
+	}, {
+		.name = "dspp_1", .id = DSPP_1,
+		.base = 0x56000, .len = 0x1800,
+		.features = DSPP_SC7180_MASK,
+		.sblk = &msm8998_dspp_sblk,
+	},
+};
+
+static const struct dpu_intf_cfg sdm660_intf[] = {
+	{
+		.name = "intf_0", .id = INTF_0,
+		.base = 0x6a000, .len = 0x280,
+		.type = INTF_DP,
+		.controller_id = MSM_DP_CONTROLLER_0,
+		.prog_fetch_lines_worst_case = 21,
+		.intr_underrun = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 24),
+		.intr_vsync = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 25),
+		.intr_tear_rd_ptr = -1,
+	}, {
+		.name = "intf_1", .id = INTF_1,
+		.base = 0x6a800, .len = 0x280,
+		.type = INTF_DSI,
+		.controller_id = MSM_DSI_CONTROLLER_0,
+		.prog_fetch_lines_worst_case = 21,
+		.intr_underrun = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 26),
+		.intr_vsync = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 27),
+		.intr_tear_rd_ptr = -1,
+	}, {
+		.name = "intf_2", .id = INTF_2,
+		.base = 0x6b000, .len = 0x280,
+		.type = INTF_DSI,
+		.controller_id = MSM_DSI_CONTROLLER_1,
+		.prog_fetch_lines_worst_case = 21,
+		.intr_underrun = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 28),
+		.intr_vsync = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 29),
+		.intr_tear_rd_ptr = -1,
+	},
+};
+
+static const struct dpu_perf_cfg sdm660_perf_data = {
+	.max_bw_low = 6600000,
+	.max_bw_high = 6600000,
+	.min_core_ib = 3100000,
+	.min_llcc_ib = 800000,
+	.min_dram_ib = 800000,
+	.undersized_prefill_lines = 2,
+	.xtra_prefill_lines = 2,
+	.dest_scale_prefill_lines = 3,
+	.macrotile_prefill_lines = 4,
+	.yuv_nv12_prefill_lines = 8,
+	.linear_prefill_lines = 1,
+	.downscaling_prefill_lines = 1,
+	.amortizable_threshold = 25,
+	.min_prefill_lines = 25,
+	.danger_lut_tbl = {0xf, 0xffff, 0x0},
+	.safe_lut_tbl = {0xfffc, 0xff00, 0xffff},
+	.qos_lut_tbl = {
+		{.nentry = ARRAY_SIZE(msm8998_qos_linear),
+		.entries = msm8998_qos_linear
+		},
+		{.nentry = ARRAY_SIZE(msm8998_qos_macrotile),
+		.entries = msm8998_qos_macrotile
+		},
+		{.nentry = ARRAY_SIZE(msm8998_qos_nrt),
+		.entries = msm8998_qos_nrt
+		},
+	},
+	.cdp_cfg = {
+		{.rd_enable = 1, .wr_enable = 1},
+		{.rd_enable = 1, .wr_enable = 0}
+	},
+	.clk_inefficiency_factor = 200,
+	.bw_inefficiency_factor = 120,
+};
+
+static const struct dpu_mdss_version sdm660_mdss_ver = {
+	.core_major_ver = 3,
+	.core_minor_ver = 2,
+};
+
+const struct dpu_mdss_cfg dpu_sdm660_cfg = {
+	.mdss_ver = &sdm660_mdss_ver,
+	.caps = &sdm660_dpu_caps,
+	.mdp = &sdm660_mdp,
+	.ctl_count = ARRAY_SIZE(sdm660_ctl),
+	.ctl = sdm660_ctl,
+	.sspp_count = ARRAY_SIZE(sdm660_sspp),
+	.sspp = sdm660_sspp,
+	.mixer_count = ARRAY_SIZE(sdm660_lm),
+	.mixer = sdm660_lm,
+	.dspp_count = ARRAY_SIZE(sdm660_dspp),
+	.dspp = sdm660_dspp,
+	.pingpong_count = ARRAY_SIZE(sdm660_pp),
+	.pingpong = sdm660_pp,
+	.dsc_count = ARRAY_SIZE(sdm660_dsc),
+	.dsc = sdm660_dsc,
+	.intf_count = ARRAY_SIZE(sdm660_intf),
+	.intf = sdm660_intf,
+	.vbif_count = ARRAY_SIZE(msm8998_vbif),
+	.vbif = msm8998_vbif,
+	.perf = &sdm660_perf_data,
+};
+
+#endif
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_3_sdm630.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_3_sdm630.h
new file mode 100644
index 000000000000..df01227fc364
--- /dev/null
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_3_sdm630.h
@@ -0,0 +1,225 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2023. Linaro Inc. All rights reserved.
+ */
+
+#ifndef _DPU_3_3_SDM630_H
+#define _DPU_3_3_SDM630_H
+
+static const struct dpu_caps sdm630_dpu_caps = {
+	.max_mixer_width = DEFAULT_DPU_LINE_WIDTH,
+	.max_mixer_blendstages = 0x7,
+	.has_src_split = true,
+	.has_dim_layer = true,
+	.has_idle_pc = true,
+	.has_3d_merge = true,
+	.max_linewidth = DEFAULT_DPU_LINE_WIDTH,
+	.pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
+	.max_hdeci_exp = MAX_HORZ_DECIMATION,
+	.max_vdeci_exp = MAX_VERT_DECIMATION,
+};
+
+static const struct dpu_mdp_cfg sdm630_mdp = {
+	.name = "top_0",
+	.base = 0x0, .len = 0x458,
+	.features = BIT(DPU_MDP_VSYNC_SEL),
+	.clk_ctrls = {
+		[DPU_CLK_CTRL_VIG0] = { .reg_off = 0x2ac, .bit_off = 0 },
+		[DPU_CLK_CTRL_DMA0] = { .reg_off = 0x2ac, .bit_off = 8 },
+		[DPU_CLK_CTRL_DMA1] = { .reg_off = 0x2b4, .bit_off = 8 },
+		[DPU_CLK_CTRL_DMA2] = { .reg_off = 0x2c4, .bit_off = 8 },
+		[DPU_CLK_CTRL_CURSOR0] = { .reg_off = 0x3a8, .bit_off = 16 },
+	},
+};
+
+static const struct dpu_ctl_cfg sdm630_ctl[] = {
+	{
+		.name = "ctl_0", .id = CTL_0,
+		.base = 0x1000, .len = 0x94,
+		.features = BIT(DPU_CTL_SPLIT_DISPLAY),
+		.intr_start = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR2, 9),
+	}, {
+		.name = "ctl_1", .id = CTL_1,
+		.base = 0x1200, .len = 0x94,
+		.intr_start = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR2, 10),
+	}, {
+		.name = "ctl_2", .id = CTL_2,
+		.base = 0x1400, .len = 0x94,
+		.features = BIT(DPU_CTL_SPLIT_DISPLAY),
+		.intr_start = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR2, 11),
+	}, {
+		.name = "ctl_3", .id = CTL_3,
+		.base = 0x1600, .len = 0x94,
+		.intr_start = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR2, 12),
+	}, {
+		.name = "ctl_4", .id = CTL_4,
+		.base = 0x1800, .len = 0x94,
+		.intr_start = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR2, 13),
+	},
+};
+
+static const struct dpu_sspp_cfg sdm630_sspp[] = {
+	{
+		.name = "sspp_0", .id = SSPP_VIG0,
+		.base = 0x4000, .len = 0x1ac,
+		.features = VIG_MSM8998_MASK,
+		.sblk = &dpu_vig_sblk_qseed3_1_2,
+		.xin_id = 0,
+		.type = SSPP_TYPE_VIG,
+		.clk_ctrl = DPU_CLK_CTRL_VIG0,
+	}, {
+		.name = "sspp_8", .id = SSPP_DMA0,
+		.base = 0x24000, .len = 0x1ac,
+		.features = DMA_MSM8998_MASK,
+		.sblk = &dpu_dma_sblk,
+		.xin_id = 1,
+		.type = SSPP_TYPE_DMA,
+		.clk_ctrl = DPU_CLK_CTRL_DMA0,
+	}, {
+		.name = "sspp_9", .id = SSPP_DMA1,
+		.base = 0x26000, .len = 0x1ac,
+		.features = DMA_MSM8998_MASK,
+		.sblk = &dpu_dma_sblk,
+		.xin_id = 5,
+		.type = SSPP_TYPE_DMA,
+		.clk_ctrl = DPU_CLK_CTRL_DMA1,
+	}, {
+		.name = "sspp_10", .id = SSPP_DMA2,
+		.base = 0x28000, .len = 0x1ac,
+		.features = DMA_CURSOR_MSM8998_MASK,
+		.sblk = &dpu_dma_sblk,
+		.xin_id = 9,
+		.type = SSPP_TYPE_DMA,
+		.clk_ctrl = DPU_CLK_CTRL_DMA2,
+	},
+};
+
+static const struct dpu_lm_cfg sdm630_lm[] = {
+	{
+		.name = "lm_0", .id = LM_0,
+		.base = 0x44000, .len = 0x320,
+		.features = MIXER_MSM8998_MASK,
+		.sblk = &msm8998_lm_sblk,
+		.pingpong = PINGPONG_0,
+		.dspp = DSPP_0,
+	}, {
+		.name = "lm_2", .id = LM_2,
+		.base = 0x46000, .len = 0x320,
+		.features = MIXER_MSM8998_MASK,
+		.sblk = &msm8998_lm_sblk,
+		.pingpong = PINGPONG_2,
+	},
+};
+
+static const struct dpu_pingpong_cfg sdm630_pp[] = {
+	{
+		.name = "pingpong_0", .id = PINGPONG_0,
+		.base = 0x70000, .len = 0xd4,
+		.features = PINGPONG_SDM845_TE2_MASK,
+		.sblk = &sdm845_pp_sblk_te,
+		.intr_done = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 8),
+		.intr_rdptr = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 12),
+	}, {
+		.name = "pingpong_2", .id = PINGPONG_2,
+		.base = 0x71000, .len = 0xd4,
+		.features = PINGPONG_SDM845_MASK,
+		.sblk = &sdm845_pp_sblk,
+		.intr_done = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 10),
+		.intr_rdptr = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 14),
+	},
+};
+
+static const struct dpu_dspp_cfg sdm630_dspp[] = {
+	{
+		.name = "dspp_0", .id = DSPP_0,
+		.base = 0x54000, .len = 0x1800,
+		.features = DSPP_SC7180_MASK,
+		.sblk = &msm8998_dspp_sblk,
+	},
+};
+
+static const struct dpu_intf_cfg sdm630_intf[] = {
+	{
+		.name = "intf_0", .id = INTF_0,
+		.base = 0x6a000, .len = 0x280,
+		.type = INTF_DP,
+		.controller_id = MSM_DP_CONTROLLER_0,
+		.prog_fetch_lines_worst_case = 21,
+		.intr_underrun = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 24),
+		.intr_vsync = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 25),
+		.intr_tear_rd_ptr = -1,
+	}, {
+		.name = "intf_1", .id = INTF_1,
+		.base = 0x6a800, .len = 0x280,
+		.type = INTF_DSI,
+		.controller_id = MSM_DSI_CONTROLLER_0,
+		.prog_fetch_lines_worst_case = 21,
+		.intr_underrun = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 26),
+		.intr_vsync = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 27),
+		.intr_tear_rd_ptr = -1,
+	},
+};
+
+static const struct dpu_perf_cfg sdm630_perf_data = {
+	.max_bw_low = 4100000,
+	.max_bw_high = 4100000,
+	.min_core_ib = 3200000,
+	.min_llcc_ib = 800000,
+	.min_dram_ib = 800000,
+	.undersized_prefill_lines = 2,
+	.xtra_prefill_lines = 2,
+	.dest_scale_prefill_lines = 3,
+	.macrotile_prefill_lines = 4,
+	.yuv_nv12_prefill_lines = 8,
+	.linear_prefill_lines = 1,
+	.downscaling_prefill_lines = 1,
+	.amortizable_threshold = 25,
+	.min_prefill_lines = 25,
+	.danger_lut_tbl = {0xf, 0xffff, 0x0},
+	.safe_lut_tbl = {0xfffc, 0xff00, 0xffff},
+	.qos_lut_tbl = {
+		{.nentry = ARRAY_SIZE(msm8998_qos_linear),
+		.entries = msm8998_qos_linear
+		},
+		{.nentry = ARRAY_SIZE(msm8998_qos_macrotile),
+		.entries = msm8998_qos_macrotile
+		},
+		{.nentry = ARRAY_SIZE(msm8998_qos_nrt),
+		.entries = msm8998_qos_nrt
+		},
+	},
+	.cdp_cfg = {
+		{.rd_enable = 1, .wr_enable = 1},
+		{.rd_enable = 1, .wr_enable = 0}
+	},
+	.clk_inefficiency_factor = 200,
+	.bw_inefficiency_factor = 120,
+};
+
+static const struct dpu_mdss_version sdm630_mdss_ver = {
+	.core_major_ver = 3,
+	.core_minor_ver = 3,
+};
+
+const struct dpu_mdss_cfg dpu_sdm630_cfg = {
+	.mdss_ver = &sdm630_mdss_ver,
+	.caps = &sdm630_dpu_caps,
+	.mdp = &sdm630_mdp,
+	.ctl_count = ARRAY_SIZE(sdm630_ctl),
+	.ctl = sdm630_ctl,
+	.sspp_count = ARRAY_SIZE(sdm630_sspp),
+	.sspp = sdm630_sspp,
+	.mixer_count = ARRAY_SIZE(sdm630_lm),
+	.mixer = sdm630_lm,
+	.dspp_count = ARRAY_SIZE(sdm630_dspp),
+	.dspp = sdm630_dspp,
+	.pingpong_count = ARRAY_SIZE(sdm630_pp),
+	.pingpong = sdm630_pp,
+	.intf_count = ARRAY_SIZE(sdm630_intf),
+	.intf = sdm630_intf,
+	.vbif_count = ARRAY_SIZE(msm8998_vbif),
+	.vbif = msm8998_vbif,
+	.perf = &sdm630_perf_data,
+};
+
+#endif
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
index 54e8717403a0..ccbee0f40ad7 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -680,6 +680,8 @@ static const struct dpu_qos_lut_entry sc7180_qos_nrt[] = {
  *************************************************************/
 
 #include "catalog/dpu_3_0_msm8998.h"
+#include "catalog/dpu_3_2_sdm660.h"
+#include "catalog/dpu_3_3_sdm630.h"
 
 #include "catalog/dpu_4_0_sdm845.h"
 #include "catalog/dpu_4_1_sdm670.h"
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 ba82ef4560a6..a54c135e3147 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
@@ -832,6 +832,8 @@ struct dpu_mdss_cfg {
 };
 
 extern const struct dpu_mdss_cfg dpu_msm8998_cfg;
+extern const struct dpu_mdss_cfg dpu_sdm630_cfg;
+extern const struct dpu_mdss_cfg dpu_sdm660_cfg;
 extern const struct dpu_mdss_cfg dpu_sdm845_cfg;
 extern const struct dpu_mdss_cfg dpu_sdm670_cfg;
 extern const struct dpu_mdss_cfg dpu_sm8150_cfg;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 8f11a98491a1..dba8786bff3e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -1377,6 +1377,8 @@ static const struct dev_pm_ops dpu_pm_ops = {
 static const struct of_device_id dpu_dt_match[] = {
 	{ .compatible = "qcom,msm8998-dpu", .data = &dpu_msm8998_cfg, },
 	{ .compatible = "qcom,qcm2290-dpu", .data = &dpu_qcm2290_cfg, },
+	{ .compatible = "qcom,sdm630-mdp5", .data = &dpu_sdm630_cfg, },
+	{ .compatible = "qcom,sdm660-mdp5", .data = &dpu_sdm660_cfg, },
 	{ .compatible = "qcom,sdm670-dpu", .data = &dpu_sdm670_cfg, },
 	{ .compatible = "qcom,sdm845-dpu", .data = &dpu_sdm845_cfg, },
 	{ .compatible = "qcom,sc7180-dpu", .data = &dpu_sc7180_cfg, },
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index ef57586fbeca..a8748a858e61 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -983,6 +983,8 @@ module_param(prefer_mdp5, bool, 0444);
 
 /* list all platforms supported by both mdp5 and dpu drivers */
 static const char *const msm_mdp5_dpu_migration[] = {
+	"qcom,sdm630-mdp5",
+	"qcom,sdm660-mdp5",
 	NULL,
 };
 

-- 
2.39.2

Re: [PATCH v3 3/4] drm/msm: add a kernel param to select between MDP5 and DPU drivers

[Carl Vanderlip]

On 1/5/2024 3:34 PM, Dmitry Baryshkov wrote:
> For some of the platforms (e.g. SDM660, SDM630, MSM8996, etc.) it is
> possible to support this platform via the DPU driver (e.g. to provide
> support for DP, multirect, etc). Add a modparam to be able to switch
> between these two drivers.
> 
> All platforms supported by both drivers are by default handled by the
> MDP5 driver. To let them be handled by the DPU driver pass the
> `msm.prefer_mdp5=false` kernel param.
> 
> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  |  3 +++
>   drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c |  3 +++
>   drivers/gpu/drm/msm/msm_drv.c            | 31 +++++++++++++++++++++++++++++++
>   drivers/gpu/drm/msm/msm_drv.h            |  1 +
>   4 files changed, 38 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index aa9e0ad33ebb..8f11a98491a1 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -1276,6 +1276,9 @@ static int dpu_dev_probe(struct platform_device *pdev)
>   	int irq;
>   	int ret = 0;
>   
> +	if (!msm_disp_drv_should_bind(&pdev->dev, true))
> +		return -ENODEV;
> +
>   	dpu_kms = devm_kzalloc(dev, sizeof(*dpu_kms), GFP_KERNEL);
>   	if (!dpu_kms)
>   		return -ENOMEM;
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> index 0827634664ae..43d05851c54d 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> @@ -866,6 +866,9 @@ static int mdp5_dev_probe(struct platform_device *pdev)
>   
>   	DBG("");
>   
> +	if (!msm_disp_drv_should_bind(&pdev->dev, false))
> +		return -ENODEV;
> +
>   	mdp5_kms = devm_kzalloc(&pdev->dev, sizeof(*mdp5_kms), GFP_KERNEL);
>   	if (!mdp5_kms)
>   		return -ENOMEM;
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 50b65ffc24b1..ef57586fbeca 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -969,6 +969,37 @@ static int add_components_mdp(struct device *master_dev,
>   	return 0;
>   }
>   
> +#if !IS_REACHABLE(CONFIG_DRM_MSM_MDP5) || !IS_REACHABLE(CONFIG_DRM_MSM_DPU)
> +bool msm_disp_drv_should_bind(struct device *dev, bool mdp5_driver)

s/mdp5_driver/dpu_driver/

> +{
> +	/* If just a single driver is enabled, use it no matter what */
> +	return true;
> +}

This will cause both MDP/DPU probes to return -ENODEV, rather than
select the enabled one.

> +#else
> +
> +static bool prefer_mdp5 = true;
> +MODULE_PARM_DESC(prefer_mdp5, "Select whether MDP5 or DPU driver should be preferred");
> +module_param(prefer_mdp5, bool, 0444);
> +
> +/* list all platforms supported by both mdp5 and dpu drivers */
> +static const char *const msm_mdp5_dpu_migration[] = {
> +	NULL,
> +};
> +
> +bool msm_disp_drv_should_bind(struct device *dev, bool dpu_driver)
> +{
> +	/* If it is not an MDP5 device, do not try MDP5 driver */
> +	if (!of_device_is_compatible(dev->of_node, "qcom,mdp5"))
> +		return dpu_driver;
> +
> +	/* If it is not in the migration list, use MDP5 */
> +	if (!of_device_compatible_match(dev->of_node, msm_mdp5_dpu_migration))
> +		return !dpu_driver;
> +
> +	return prefer_mdp5 ? !dpu_driver : dpu_driver;
> +}
> +#endif
> +
>   /*
>    * We don't know what's the best binding to link the gpu with the drm device.
>    * Fow now, we just hunt for all the possible gpus that we support, and add them
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index 01e783130054..762e13e2df74 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -563,5 +563,6 @@ int msm_drv_probe(struct device *dev,
>   	struct msm_kms *kms);
>   void msm_kms_shutdown(struct platform_device *pdev);
>   
> +bool msm_disp_drv_should_bind(struct device *dev, bool dpu_driver);
>   
>   #endif /* __MSM_DRV_H__ */
> 

Re: [PATCH v3 3/4] drm/msm: add a kernel param to select between MDP5 and DPU drivers

[Dmitry Baryshkov]

On Sat, 6 Jan 2024 at 02:04, Carl Vanderlip <quic_carlv@quicinc.com> wrote:
>
>
> On 1/5/2024 3:34 PM, Dmitry Baryshkov wrote:
> > For some of the platforms (e.g. SDM660, SDM630, MSM8996, etc.) it is
> > possible to support this platform via the DPU driver (e.g. to provide
> > support for DP, multirect, etc). Add a modparam to be able to switch
> > between these two drivers.
> >
> > All platforms supported by both drivers are by default handled by the
> > MDP5 driver. To let them be handled by the DPU driver pass the
> > `msm.prefer_mdp5=false` kernel param.
> >
> > Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  |  3 +++
> >   drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c |  3 +++
> >   drivers/gpu/drm/msm/msm_drv.c            | 31 +++++++++++++++++++++++++++++++
> >   drivers/gpu/drm/msm/msm_drv.h            |  1 +
> >   4 files changed, 38 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > index aa9e0ad33ebb..8f11a98491a1 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > @@ -1276,6 +1276,9 @@ static int dpu_dev_probe(struct platform_device *pdev)
> >       int irq;
> >       int ret = 0;
> >
> > +     if (!msm_disp_drv_should_bind(&pdev->dev, true))
> > +             return -ENODEV;
> > +
> >       dpu_kms = devm_kzalloc(dev, sizeof(*dpu_kms), GFP_KERNEL);
> >       if (!dpu_kms)
> >               return -ENOMEM;
> > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> > index 0827634664ae..43d05851c54d 100644
> > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> > @@ -866,6 +866,9 @@ static int mdp5_dev_probe(struct platform_device *pdev)
> >
> >       DBG("");
> >
> > +     if (!msm_disp_drv_should_bind(&pdev->dev, false))
> > +             return -ENODEV;
> > +
> >       mdp5_kms = devm_kzalloc(&pdev->dev, sizeof(*mdp5_kms), GFP_KERNEL);
> >       if (!mdp5_kms)
> >               return -ENOMEM;
> > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> > index 50b65ffc24b1..ef57586fbeca 100644
> > --- a/drivers/gpu/drm/msm/msm_drv.c
> > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > @@ -969,6 +969,37 @@ static int add_components_mdp(struct device *master_dev,
> >       return 0;
> >   }
> >
> > +#if !IS_REACHABLE(CONFIG_DRM_MSM_MDP5) || !IS_REACHABLE(CONFIG_DRM_MSM_DPU)
> > +bool msm_disp_drv_should_bind(struct device *dev, bool mdp5_driver)
>
> s/mdp5_driver/dpu_driver/

Well, ignored_driver, but your suggestion is better.

>
> > +{
> > +     /* If just a single driver is enabled, use it no matter what */
> > +     return true;
> > +}
>
> This will cause both MDP/DPU probes to return -ENODEV, rather than
> select the enabled one.

No. The code (e.g. for DPU) is:

       if (!msm_disp_drv_should_bind(&pdev->dev, true))
                return -ENODEV;

So the driver returns -ENODEV if msm_disp_drv_should_bind() returns
false. Which is logical from the function name point of view.

>
> > +#else
> > +
> > +static bool prefer_mdp5 = true;
> > +MODULE_PARM_DESC(prefer_mdp5, "Select whether MDP5 or DPU driver should be preferred");
> > +module_param(prefer_mdp5, bool, 0444);
> > +
> > +/* list all platforms supported by both mdp5 and dpu drivers */
> > +static const char *const msm_mdp5_dpu_migration[] = {
> > +     NULL,
> > +};
> > +
> > +bool msm_disp_drv_should_bind(struct device *dev, bool dpu_driver)
> > +{
> > +     /* If it is not an MDP5 device, do not try MDP5 driver */
> > +     if (!of_device_is_compatible(dev->of_node, "qcom,mdp5"))
> > +             return dpu_driver;
> > +
> > +     /* If it is not in the migration list, use MDP5 */
> > +     if (!of_device_compatible_match(dev->of_node, msm_mdp5_dpu_migration))
> > +             return !dpu_driver;
> > +
> > +     return prefer_mdp5 ? !dpu_driver : dpu_driver;
> > +}
> > +#endif
> > +
> >   /*
> >    * We don't know what's the best binding to link the gpu with the drm device.
> >    * Fow now, we just hunt for all the possible gpus that we support, and add them
> > diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> > index 01e783130054..762e13e2df74 100644
> > --- a/drivers/gpu/drm/msm/msm_drv.h
> > +++ b/drivers/gpu/drm/msm/msm_drv.h
> > @@ -563,5 +563,6 @@ int msm_drv_probe(struct device *dev,
> >       struct msm_kms *kms);
> >   void msm_kms_shutdown(struct platform_device *pdev);
> >
> > +bool msm_disp_drv_should_bind(struct device *dev, bool dpu_driver);
> >
> >   #endif /* __MSM_DRV_H__ */
> >



-- 
With best wishes
Dmitry

Re: [PATCH v3 3/4] drm/msm: add a kernel param to select between MDP5 and DPU drivers

[Carl Vanderlip]

On 1/5/2024 4:38 PM, Dmitry Baryshkov wrote:
> On Sat, 6 Jan 2024 at 02:04, Carl Vanderlip <quic_carlv@quicinc.com> wrote:
>>
>>
>> On 1/5/2024 3:34 PM, Dmitry Baryshkov wrote:
>>> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
>>> index 50b65ffc24b1..ef57586fbeca 100644
>>> --- a/drivers/gpu/drm/msm/msm_drv.c
>>> +++ b/drivers/gpu/drm/msm/msm_drv.c
>>> @@ -969,6 +969,37 @@ static int add_components_mdp(struct device *master_dev,
>>>        return 0;
>>>    }
>>>
>>> +#if !IS_REACHABLE(CONFIG_DRM_MSM_MDP5) || !IS_REACHABLE(CONFIG_DRM_MSM_DPU)
>>> +bool msm_disp_drv_should_bind(struct device *dev, bool mdp5_driver)
>>> +{
>>> +     /* If just a single driver is enabled, use it no matter what */
>>> +     return true;
>>> +}
>>
>> This will cause both MDP/DPU probes to return -ENODEV, rather than
>> select the enabled one.
> 
> No. The code (e.g. for DPU) is:
> 
>         if (!msm_disp_drv_should_bind(&pdev->dev, true))
>                  return -ENODEV;
> 
> So the driver returns -ENODEV if msm_disp_drv_should_bind() returns
> false. Which is logical from the function name point of view.
> 

but msm_disp_drv_should_bind() is returning true in the #if !REACHABLE() 
case?

at minimum the comment is incorrect since returning true causes the
driver to NOT be used.

-Carl V.

Re: [PATCH v3 3/4] drm/msm: add a kernel param to select between MDP5 and DPU drivers

[Dmitry Baryshkov]

On Mon, 8 Jan 2024 at 19:57, Carl Vanderlip <quic_carlv@quicinc.com> wrote:
>
>
>
> On 1/5/2024 4:38 PM, Dmitry Baryshkov wrote:
> > On Sat, 6 Jan 2024 at 02:04, Carl Vanderlip <quic_carlv@quicinc.com> wrote:
> >>
> >>
> >> On 1/5/2024 3:34 PM, Dmitry Baryshkov wrote:
> >>> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> >>> index 50b65ffc24b1..ef57586fbeca 100644
> >>> --- a/drivers/gpu/drm/msm/msm_drv.c
> >>> +++ b/drivers/gpu/drm/msm/msm_drv.c
> >>> @@ -969,6 +969,37 @@ static int add_components_mdp(struct device *master_dev,
> >>>        return 0;
> >>>    }
> >>>
> >>> +#if !IS_REACHABLE(CONFIG_DRM_MSM_MDP5) || !IS_REACHABLE(CONFIG_DRM_MSM_DPU)
> >>> +bool msm_disp_drv_should_bind(struct device *dev, bool mdp5_driver)
> >>> +{
> >>> +     /* If just a single driver is enabled, use it no matter what */
> >>> +     return true;
> >>> +}
> >>
> >> This will cause both MDP/DPU probes to return -ENODEV, rather than
> >> select the enabled one.
> >
> > No. The code (e.g. for DPU) is:
> >
> >         if (!msm_disp_drv_should_bind(&pdev->dev, true))
> >                  return -ENODEV;
> >
> > So the driver returns -ENODEV if msm_disp_drv_should_bind() returns
> > false. Which is logical from the function name point of view.
> >
>
> but msm_disp_drv_should_bind() is returning true in the #if !REACHABLE()
> case?
>
> at minimum the comment is incorrect since returning true causes the
> driver to NOT be used.

No. Returning _false_ causes the driver to not be used.

-- 
With best wishes
Dmitry

Re: [PATCH v3 3/4] drm/msm: add a kernel param to select between MDP5 and DPU drivers

[Carl Vanderlip]

On 1/8/2024 11:07 AM, Dmitry Baryshkov wrote:
> On Mon, 8 Jan 2024 at 19:57, Carl Vanderlip <quic_carlv@quicinc.com> wrote:
>> On 1/5/2024 4:38 PM, Dmitry Baryshkov wrote:
>>> On Sat, 6 Jan 2024 at 02:04, Carl Vanderlip <quic_carlv@quicinc.com> wrote:
>>>> On 1/5/2024 3:34 PM, Dmitry Baryshkov wrote:
>>>>> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
>>>>> index 50b65ffc24b1..ef57586fbeca 100644
>>>>> --- a/drivers/gpu/drm/msm/msm_drv.c
>>>>> +++ b/drivers/gpu/drm/msm/msm_drv.c
>>>>> @@ -969,6 +969,37 @@ static int add_components_mdp(struct device *master_dev,
>>>>>         return 0;
>>>>>     }
>>>>>
>>>>> +#if !IS_REACHABLE(CONFIG_DRM_MSM_MDP5) || !IS_REACHABLE(CONFIG_DRM_MSM_DPU)
>>>>> +bool msm_disp_drv_should_bind(struct device *dev, bool mdp5_driver)
>>>>> +{
>>>>> +     /* If just a single driver is enabled, use it no matter what */
>>>>> +     return true;
>>>>> +}
>>>>
>>>> This will cause both MDP/DPU probes to return -ENODEV, rather than
>>>> select the enabled one.
>>>
>>> No. The code (e.g. for DPU) is:
>>>
>>>          if (!msm_disp_drv_should_bind(&pdev->dev, true))
>>>                   return -ENODEV;
>>>
>>> So the driver returns -ENODEV if msm_disp_drv_should_bind() returns
>>> false. Which is logical from the function name point of view.
>>>
>>
>> but msm_disp_drv_should_bind() is returning true in the #if !REACHABLE()
>> case?
>>
>> at minimum the comment is incorrect since returning true causes the
>> driver to NOT be used.
> 
> No. Returning _false_ causes the driver to not be used.
> 

Apologies... you are correct.

Reviewed-by: Carl Vanderlip <quic_carlv@quicinc.com>

-Carl V.

Re: [PATCH v3 1/4] drm/msm/mdss: generate MDSS data for MDP5 platforms

[Abhinav Kumar]

On 1/5/2024 3:34 PM, Dmitry Baryshkov wrote:
> Older (mdp5) platforms do not use per-SoC compatible strings. Instead
> they use a single compat entry 'qcom,mdss'. To facilitate migrating
> these platforms to the DPU driver provide a way to generate the MDSS /
> UBWC data at runtime, when the DPU driver asks for it.
> 
> It is not possible to generate this data structure at the probe time,
> since some platforms might not have MDP_CLK enabled, which makes reading
> HW_REV register return 0.
> 

I would have expected a crash if clock was not enabled and we tried to 
access the hw_rev register.

> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/msm_mdss.c | 51 ++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 51 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
> index 455b2e3a0cdd..566a5dd5b8e8 100644
> --- a/drivers/gpu/drm/msm/msm_mdss.c
> +++ b/drivers/gpu/drm/msm/msm_mdss.c
> @@ -3,6 +3,7 @@
>    * Copyright (c) 2018, The Linux Foundation
>    */
>   
> +#include <linux/bitfield.h>
>   #include <linux/clk.h>
>   #include <linux/delay.h>
>   #include <linux/interconnect.h>
> @@ -213,6 +214,49 @@ static void msm_mdss_setup_ubwc_dec_40(struct msm_mdss *msm_mdss)
>   	}
>   }
>   
> +#define MDSS_HW_MAJ_MIN		GENMASK(31, 16)
> +
> +#define MDSS_HW_MSM8996		0x1007
> +#define MDSS_HW_MSM8937		0x100e
> +#define MDSS_HW_MSM8956		0x1010

This should be 0x100B in the docs I see.

> +#define MDSS_HW_MSM8998		0x3000
> +#define MDSS_HW_SDM660		0x3002
> +#define MDSS_HW_SDM630		0x3003
> +
> +/*
> + * MDP5 platforms use generic qcom,mdp5 compat string, so we have to generate this data
> + */
> +static const struct msm_mdss_data *msm_mdss_generate_mdp5_mdss_data(struct msm_mdss *mdss)
> +{
> +	struct msm_mdss_data *data;
> +	u32 hw_rev;
> +
> +	data = devm_kzalloc(mdss->dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return NULL;
> +
> +	hw_rev = readl_relaxed(mdss->mmio + HW_REV);
> +	hw_rev = FIELD_GET(MDSS_HW_MAJ_MIN, hw_rev);
> +
> +	if (hw_rev == MDSS_HW_MSM8996 ||
> +	    hw_rev == MDSS_HW_MSM8937 ||
> +	    hw_rev == MDSS_HW_MSM8956 ||
> +	    hw_rev == MDSS_HW_MSM8998 ||
> +	    hw_rev == MDSS_HW_SDM660 ||
> +	    hw_rev == MDSS_HW_SDM630) {
> +		data->ubwc_dec_version = UBWC_1_0;
> +		data->ubwc_enc_version = UBWC_1_0;
> +	}
> +
> +	if (hw_rev == MDSS_HW_MSM8996 ||
> +	    hw_rev == MDSS_HW_MSM8998)
> +		data->highest_bank_bit = 2;
> +	else
> +		data->highest_bank_bit = 1;
> +
> +	return data;
> +}
> +
>   const struct msm_mdss_data *msm_mdss_get_mdss_data(struct device *dev)
>   {
>   	struct msm_mdss *mdss;
> @@ -222,6 +266,13 @@ const struct msm_mdss_data *msm_mdss_get_mdss_data(struct device *dev)
>   
>   	mdss = dev_get_drvdata(dev);
>   
> +	/*
> +	 * We could not do it at the probe time, since hw revision register was
> +	 * not readable. Fill data structure now for the MDP5 platforms.
> +	 */
> +	if (!mdss->mdss_data && mdss->is_mdp5)
> +		mdss->mdss_data = msm_mdss_generate_mdp5_mdss_data(mdss);
> +
>   	return mdss->mdss_data;
>   }
>   
> 

Re: [PATCH v3 2/4] drm/msm/dpu: support binding to the mdp5 devices

[Abhinav Kumar]

On 1/5/2024 3:34 PM, Dmitry Baryshkov wrote:
> Existing MDP5 devices have slightly different bindings. The main
> register region is called `mdp_phys' instead of `mdp'. Also vbif
> register regions are a part of the parent, MDSS device. Add support for
> handling this binding differences.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 98 ++++++++++++++++++++++++++-------
>   drivers/gpu/drm/msm/msm_drv.h           |  3 +
>   drivers/gpu/drm/msm/msm_io_utils.c      | 13 +++++
>   3 files changed, 93 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 723cc1d82143..aa9e0ad33ebb 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -1197,6 +1197,78 @@ static int dpu_kms_init(struct drm_device *ddev)
>   	return 0;
>   }
>   
> +static int dpu_kms_mmap_mdp5(struct dpu_kms *dpu_kms)
> +{
> +	struct platform_device *pdev = dpu_kms->pdev;
> +	struct platform_device *mdss_dev;
> +	int ret;
> +
> +	if (dpu_kms->pdev->dev.bus != &platform_bus_type)
> +		return -EINVAL;
> +

!dev_is_platform() perhaps?

But I would like to understand this check a bit more. Can you pls 
explain for which case this check was added?

> +	mdss_dev = to_platform_device(dpu_kms->pdev->dev.parent);
> +
> +	dpu_kms->mmio = msm_ioremap(pdev, "mdp_phys");
> +	if (IS_ERR(dpu_kms->mmio)) {
> +		ret = PTR_ERR(dpu_kms->mmio);
> +		DPU_ERROR("mdp register memory map failed: %d\n", ret);
> +		dpu_kms->mmio = NULL;
> +		return ret;
> +	}
> +	DRM_DEBUG("mapped dpu address space @%pK\n", dpu_kms->mmio);
> +
> +	dpu_kms->vbif[VBIF_RT] = msm_ioremap_mdss(mdss_dev,
> +						  dpu_kms->pdev,
> +						  "vbif_phys");
> +	if (IS_ERR(dpu_kms->vbif[VBIF_RT])) {
> +		ret = PTR_ERR(dpu_kms->vbif[VBIF_RT]);
> +		DPU_ERROR("vbif register memory map failed: %d\n", ret);
> +		dpu_kms->vbif[VBIF_RT] = NULL;
> +		return ret;
> +	}
> +
> +	dpu_kms->vbif[VBIF_NRT] = msm_ioremap_mdss(mdss_dev,
> +						   dpu_kms->pdev,
> +						   "vbif_nrt_phys");

Do you think a "quiet" version would be better?


> +	if (IS_ERR(dpu_kms->vbif[VBIF_NRT])) {
> +		dpu_kms->vbif[VBIF_NRT] = NULL;
> +		DPU_DEBUG("VBIF NRT is not defined");
> +	}
> +
> +	return 0;
> +}
> +
> +static int dpu_kms_mmap_dpu(struct dpu_kms *dpu_kms)
> +{
> +	struct platform_device *pdev = dpu_kms->pdev;
> +	int ret;
> +
> +	dpu_kms->mmio = msm_ioremap(pdev, "mdp");
> +	if (IS_ERR(dpu_kms->mmio)) {
> +		ret = PTR_ERR(dpu_kms->mmio);
> +		DPU_ERROR("mdp register memory map failed: %d\n", ret);
> +		dpu_kms->mmio = NULL;
> +		return ret;
> +	}
> +	DRM_DEBUG("mapped dpu address space @%pK\n", dpu_kms->mmio);
> +
> +	dpu_kms->vbif[VBIF_RT] = msm_ioremap(pdev, "vbif");
> +	if (IS_ERR(dpu_kms->vbif[VBIF_RT])) {
> +		ret = PTR_ERR(dpu_kms->vbif[VBIF_RT]);
> +		DPU_ERROR("vbif register memory map failed: %d\n", ret);
> +		dpu_kms->vbif[VBIF_RT] = NULL;
> +		return ret;
> +	}
> +
> +	dpu_kms->vbif[VBIF_NRT] = msm_ioremap_quiet(pdev, "vbif_nrt");
> +	if (IS_ERR(dpu_kms->vbif[VBIF_NRT])) {
> +		dpu_kms->vbif[VBIF_NRT] = NULL;
> +		DPU_DEBUG("VBIF NRT is not defined");
> +	}
> +
> +	return 0;
> +}
> +
>   static int dpu_dev_probe(struct platform_device *pdev)
>   {
>   	struct device *dev = &pdev->dev;
> @@ -1230,28 +1302,12 @@ static int dpu_dev_probe(struct platform_device *pdev)
>   
>   	dpu_kms->base.irq = irq;
>   
> -	dpu_kms->mmio = msm_ioremap(pdev, "mdp");
> -	if (IS_ERR(dpu_kms->mmio)) {
> -		ret = PTR_ERR(dpu_kms->mmio);
> -		DPU_ERROR("mdp register memory map failed: %d\n", ret);
> -		dpu_kms->mmio = NULL;
> -		return ret;
> -	}
> -	DRM_DEBUG("mapped dpu address space @%pK\n", dpu_kms->mmio);
> -
> -	dpu_kms->vbif[VBIF_RT] = msm_ioremap(pdev, "vbif");
> -	if (IS_ERR(dpu_kms->vbif[VBIF_RT])) {
> -		ret = PTR_ERR(dpu_kms->vbif[VBIF_RT]);
> -		DPU_ERROR("vbif register memory map failed: %d\n", ret);
> -		dpu_kms->vbif[VBIF_RT] = NULL;
> +	if (of_device_is_compatible(dpu_kms->pdev->dev.of_node, "qcom,mdp5"))
> +		ret = dpu_kms_mmap_mdp5(dpu_kms);
> +	else
> +		ret = dpu_kms_mmap_dpu(dpu_kms);
> +	if (ret)
>   		return ret;
> -	}
> -
> -	dpu_kms->vbif[VBIF_NRT] = msm_ioremap_quiet(pdev, "vbif_nrt");
> -	if (IS_ERR(dpu_kms->vbif[VBIF_NRT])) {
> -		dpu_kms->vbif[VBIF_NRT] = NULL;
> -		DPU_DEBUG("VBIF NRT is not defined");
> -	}
>   
>   	ret = dpu_kms_parse_data_bus_icc_path(dpu_kms);
>   	if (ret)
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index 16a7cbc0b7dd..01e783130054 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -476,6 +476,9 @@ void __iomem *msm_ioremap(struct platform_device *pdev, const char *name);
>   void __iomem *msm_ioremap_size(struct platform_device *pdev, const char *name,
>   		phys_addr_t *size);
>   void __iomem *msm_ioremap_quiet(struct platform_device *pdev, const char *name);
> +void __iomem *msm_ioremap_mdss(struct platform_device *mdss_pdev,
> +			       struct platform_device *dev,
> +			       const char *name);
>   
>   struct icc_path *msm_icc_get(struct device *dev, const char *name);
>   
> diff --git a/drivers/gpu/drm/msm/msm_io_utils.c b/drivers/gpu/drm/msm/msm_io_utils.c
> index 59d2788c4510..afedd61c3e28 100644
> --- a/drivers/gpu/drm/msm/msm_io_utils.c
> +++ b/drivers/gpu/drm/msm/msm_io_utils.c
> @@ -50,6 +50,19 @@ struct clk *msm_clk_get(struct platform_device *pdev, const char *name)
>   	return clk;
>   }
>   
> +void __iomem *msm_ioremap_mdss(struct platform_device *mdss_pdev,
> +			       struct platform_device *pdev,
> +			       const char *name)
> +{
> +	struct resource *res;
> +
> +	res = platform_get_resource_byname(mdss_pdev, IORESOURCE_MEM, name);
> +	if (!res)
> +		return ERR_PTR(-EINVAL);
> +
> +	return devm_ioremap_resource(&pdev->dev, res);
> +}
> +
>   static void __iomem *_msm_ioremap(struct platform_device *pdev, const char *name,
>   				  bool quiet, phys_addr_t *psize)
>   {
> 

Re: [PATCH v3 3/4] drm/msm: add a kernel param to select between MDP5 and DPU drivers

[Abhinav Kumar]

On 1/5/2024 3:34 PM, Dmitry Baryshkov wrote:
> For some of the platforms (e.g. SDM660, SDM630, MSM8996, etc.) it is
> possible to support this platform via the DPU driver (e.g. to provide
> support for DP, multirect, etc). Add a modparam to be able to switch
> between these two drivers.
> 
> All platforms supported by both drivers are by default handled by the
> MDP5 driver. To let them be handled by the DPU driver pass the
> `msm.prefer_mdp5=false` kernel param.
> 
> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---

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

Re: [PATCH v3 4/4] drm/msm/dpu: add support for SDM660 and SDM630 platforms

[Abhinav Kumar]

On 1/5/2024 3:34 PM, Dmitry Baryshkov wrote:
> Bring in hardware support for the SDM660 and SDM630 platforms, which
> belong to the same DPU generation as MSM8998.
> 
> Note, by default these platforms are still handled by the MDP5 driver
> unless the `msm.prefer_mdp5=false' parameter is provided.
> 
> Co-developed-by: Konrad Dybcio <konrad.dybcio@somainline.org>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@somainline.org>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   .../gpu/drm/msm/disp/dpu1/catalog/dpu_3_2_sdm660.h | 291 +++++++++++++++++++++
>   .../gpu/drm/msm/disp/dpu1/catalog/dpu_3_3_sdm630.h | 225 ++++++++++++++++
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c     |   2 +
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h     |   2 +
>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c            |   2 +
>   drivers/gpu/drm/msm/msm_drv.c                      |   2 +
>   6 files changed, 524 insertions(+)
> 

I cross-checked a few entries with the downstream sources but certainly 
not all, but based on whatever I checked all the entries were correct in 
the catalog.

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

Re: [PATCH v3 2/4] drm/msm/dpu: support binding to the mdp5 devices

[Dmitry Baryshkov]

On Wed, 7 Feb 2024 at 20:48, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 1/5/2024 3:34 PM, Dmitry Baryshkov wrote:
> > Existing MDP5 devices have slightly different bindings. The main
> > register region is called `mdp_phys' instead of `mdp'. Also vbif
> > register regions are a part of the parent, MDSS device. Add support for
> > handling this binding differences.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 98 ++++++++++++++++++++++++++-------
> >   drivers/gpu/drm/msm/msm_drv.h           |  3 +
> >   drivers/gpu/drm/msm/msm_io_utils.c      | 13 +++++
> >   3 files changed, 93 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > index 723cc1d82143..aa9e0ad33ebb 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > @@ -1197,6 +1197,78 @@ static int dpu_kms_init(struct drm_device *ddev)
> >       return 0;
> >   }
> >
> > +static int dpu_kms_mmap_mdp5(struct dpu_kms *dpu_kms)
> > +{
> > +     struct platform_device *pdev = dpu_kms->pdev;
> > +     struct platform_device *mdss_dev;
> > +     int ret;
> > +
> > +     if (dpu_kms->pdev->dev.bus != &platform_bus_type)
> > +             return -EINVAL;
> > +
>
> !dev_is_platform() perhaps?

looks good

>
> But I would like to understand this check a bit more. Can you pls
> explain for which case this check was added?

It is necessary to be sure that we can perform to_platform_device() on
the next line.

>
> > +     mdss_dev = to_platform_device(dpu_kms->pdev->dev.parent);
> > +
> > +     dpu_kms->mmio = msm_ioremap(pdev, "mdp_phys");
> > +     if (IS_ERR(dpu_kms->mmio)) {
> > +             ret = PTR_ERR(dpu_kms->mmio);
> > +             DPU_ERROR("mdp register memory map failed: %d\n", ret);
> > +             dpu_kms->mmio = NULL;
> > +             return ret;
> > +     }
> > +     DRM_DEBUG("mapped dpu address space @%pK\n", dpu_kms->mmio);
> > +
> > +     dpu_kms->vbif[VBIF_RT] = msm_ioremap_mdss(mdss_dev,
> > +                                               dpu_kms->pdev,
> > +                                               "vbif_phys");
> > +     if (IS_ERR(dpu_kms->vbif[VBIF_RT])) {
> > +             ret = PTR_ERR(dpu_kms->vbif[VBIF_RT]);
> > +             DPU_ERROR("vbif register memory map failed: %d\n", ret);
> > +             dpu_kms->vbif[VBIF_RT] = NULL;
> > +             return ret;
> > +     }
> > +
> > +     dpu_kms->vbif[VBIF_NRT] = msm_ioremap_mdss(mdss_dev,
> > +                                                dpu_kms->pdev,
> > +                                                "vbif_nrt_phys");
>
> Do you think a "quiet" version would be better?

Yep, why not.

>
>
> > +     if (IS_ERR(dpu_kms->vbif[VBIF_NRT])) {
> > +             dpu_kms->vbif[VBIF_NRT] = NULL;
> > +             DPU_DEBUG("VBIF NRT is not defined");
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int dpu_kms_mmap_dpu(struct dpu_kms *dpu_kms)
> > +{
> > +     struct platform_device *pdev = dpu_kms->pdev;
> > +     int ret;
> > +
> > +     dpu_kms->mmio = msm_ioremap(pdev, "mdp");
> > +     if (IS_ERR(dpu_kms->mmio)) {
> > +             ret = PTR_ERR(dpu_kms->mmio);
> > +             DPU_ERROR("mdp register memory map failed: %d\n", ret);
> > +             dpu_kms->mmio = NULL;
> > +             return ret;
> > +     }
> > +     DRM_DEBUG("mapped dpu address space @%pK\n", dpu_kms->mmio);
> > +
> > +     dpu_kms->vbif[VBIF_RT] = msm_ioremap(pdev, "vbif");
> > +     if (IS_ERR(dpu_kms->vbif[VBIF_RT])) {
> > +             ret = PTR_ERR(dpu_kms->vbif[VBIF_RT]);
> > +             DPU_ERROR("vbif register memory map failed: %d\n", ret);
> > +             dpu_kms->vbif[VBIF_RT] = NULL;
> > +             return ret;
> > +     }
> > +
> > +     dpu_kms->vbif[VBIF_NRT] = msm_ioremap_quiet(pdev, "vbif_nrt");
> > +     if (IS_ERR(dpu_kms->vbif[VBIF_NRT])) {
> > +             dpu_kms->vbif[VBIF_NRT] = NULL;
> > +             DPU_DEBUG("VBIF NRT is not defined");
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> >   static int dpu_dev_probe(struct platform_device *pdev)
> >   {
> >       struct device *dev = &pdev->dev;
> > @@ -1230,28 +1302,12 @@ static int dpu_dev_probe(struct platform_device *pdev)
> >
> >       dpu_kms->base.irq = irq;
> >
> > -     dpu_kms->mmio = msm_ioremap(pdev, "mdp");
> > -     if (IS_ERR(dpu_kms->mmio)) {
> > -             ret = PTR_ERR(dpu_kms->mmio);
> > -             DPU_ERROR("mdp register memory map failed: %d\n", ret);
> > -             dpu_kms->mmio = NULL;
> > -             return ret;
> > -     }
> > -     DRM_DEBUG("mapped dpu address space @%pK\n", dpu_kms->mmio);
> > -
> > -     dpu_kms->vbif[VBIF_RT] = msm_ioremap(pdev, "vbif");
> > -     if (IS_ERR(dpu_kms->vbif[VBIF_RT])) {
> > -             ret = PTR_ERR(dpu_kms->vbif[VBIF_RT]);
> > -             DPU_ERROR("vbif register memory map failed: %d\n", ret);
> > -             dpu_kms->vbif[VBIF_RT] = NULL;
> > +     if (of_device_is_compatible(dpu_kms->pdev->dev.of_node, "qcom,mdp5"))
> > +             ret = dpu_kms_mmap_mdp5(dpu_kms);
> > +     else
> > +             ret = dpu_kms_mmap_dpu(dpu_kms);
> > +     if (ret)
> >               return ret;
> > -     }
> > -
> > -     dpu_kms->vbif[VBIF_NRT] = msm_ioremap_quiet(pdev, "vbif_nrt");
> > -     if (IS_ERR(dpu_kms->vbif[VBIF_NRT])) {
> > -             dpu_kms->vbif[VBIF_NRT] = NULL;
> > -             DPU_DEBUG("VBIF NRT is not defined");
> > -     }
> >
> >       ret = dpu_kms_parse_data_bus_icc_path(dpu_kms);
> >       if (ret)
> > diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> > index 16a7cbc0b7dd..01e783130054 100644
> > --- a/drivers/gpu/drm/msm/msm_drv.h
> > +++ b/drivers/gpu/drm/msm/msm_drv.h
> > @@ -476,6 +476,9 @@ void __iomem *msm_ioremap(struct platform_device *pdev, const char *name);
> >   void __iomem *msm_ioremap_size(struct platform_device *pdev, const char *name,
> >               phys_addr_t *size);
> >   void __iomem *msm_ioremap_quiet(struct platform_device *pdev, const char *name);
> > +void __iomem *msm_ioremap_mdss(struct platform_device *mdss_pdev,
> > +                            struct platform_device *dev,
> > +                            const char *name);
> >
> >   struct icc_path *msm_icc_get(struct device *dev, const char *name);
> >
> > diff --git a/drivers/gpu/drm/msm/msm_io_utils.c b/drivers/gpu/drm/msm/msm_io_utils.c
> > index 59d2788c4510..afedd61c3e28 100644
> > --- a/drivers/gpu/drm/msm/msm_io_utils.c
> > +++ b/drivers/gpu/drm/msm/msm_io_utils.c
> > @@ -50,6 +50,19 @@ struct clk *msm_clk_get(struct platform_device *pdev, const char *name)
> >       return clk;
> >   }
> >
> > +void __iomem *msm_ioremap_mdss(struct platform_device *mdss_pdev,
> > +                            struct platform_device *pdev,
> > +                            const char *name)
> > +{
> > +     struct resource *res;
> > +
> > +     res = platform_get_resource_byname(mdss_pdev, IORESOURCE_MEM, name);
> > +     if (!res)
> > +             return ERR_PTR(-EINVAL);
> > +
> > +     return devm_ioremap_resource(&pdev->dev, res);
> > +}
> > +
> >   static void __iomem *_msm_ioremap(struct platform_device *pdev, const char *name,
> >                                 bool quiet, phys_addr_t *psize)
> >   {
> >



-- 
With best wishes
Dmitry

Re: [PATCH v3 1/4] drm/msm/mdss: generate MDSS data for MDP5 platforms

[Dmitry Baryshkov]

On Wed, 7 Feb 2024 at 20:25, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 1/5/2024 3:34 PM, Dmitry Baryshkov wrote:
> > Older (mdp5) platforms do not use per-SoC compatible strings. Instead
> > they use a single compat entry 'qcom,mdss'. To facilitate migrating
> > these platforms to the DPU driver provide a way to generate the MDSS /
> > UBWC data at runtime, when the DPU driver asks for it.
> >
> > It is not possible to generate this data structure at the probe time,
> > since some platforms might not have MDP_CLK enabled, which makes reading
> > HW_REV register return 0.
> >
>
> I would have expected a crash if clock was not enabled and we tried to
> access the hw_rev register.

No, for all the platforms I tested it returns 0 instead.
However this doesn't make any difference, we don't read the register
in MDP5 case until all clocks are enabled.

>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >   drivers/gpu/drm/msm/msm_mdss.c | 51 ++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 51 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
> > index 455b2e3a0cdd..566a5dd5b8e8 100644
> > --- a/drivers/gpu/drm/msm/msm_mdss.c
> > +++ b/drivers/gpu/drm/msm/msm_mdss.c
> > @@ -3,6 +3,7 @@
> >    * Copyright (c) 2018, The Linux Foundation
> >    */
> >
> > +#include <linux/bitfield.h>
> >   #include <linux/clk.h>
> >   #include <linux/delay.h>
> >   #include <linux/interconnect.h>
> > @@ -213,6 +214,49 @@ static void msm_mdss_setup_ubwc_dec_40(struct msm_mdss *msm_mdss)
> >       }
> >   }
> >
> > +#define MDSS_HW_MAJ_MIN              GENMASK(31, 16)
> > +
> > +#define MDSS_HW_MSM8996              0x1007
> > +#define MDSS_HW_MSM8937              0x100e
> > +#define MDSS_HW_MSM8956              0x1010
>
> This should be 0x100B in the docs I see.

Yes, I mixed MSM8956 and MSM8953 here. The code support the latter one.

>
> > +#define MDSS_HW_MSM8998              0x3000
> > +#define MDSS_HW_SDM660               0x3002
> > +#define MDSS_HW_SDM630               0x3003
> > +
> > +/*
> > + * MDP5 platforms use generic qcom,mdp5 compat string, so we have to generate this data
> > + */
> > +static const struct msm_mdss_data *msm_mdss_generate_mdp5_mdss_data(struct msm_mdss *mdss)
> > +{
> > +     struct msm_mdss_data *data;
> > +     u32 hw_rev;
> > +
> > +     data = devm_kzalloc(mdss->dev, sizeof(*data), GFP_KERNEL);
> > +     if (!data)
> > +             return NULL;
> > +
> > +     hw_rev = readl_relaxed(mdss->mmio + HW_REV);
> > +     hw_rev = FIELD_GET(MDSS_HW_MAJ_MIN, hw_rev);
> > +
> > +     if (hw_rev == MDSS_HW_MSM8996 ||
> > +         hw_rev == MDSS_HW_MSM8937 ||
> > +         hw_rev == MDSS_HW_MSM8956 ||
> > +         hw_rev == MDSS_HW_MSM8998 ||
> > +         hw_rev == MDSS_HW_SDM660 ||
> > +         hw_rev == MDSS_HW_SDM630) {
> > +             data->ubwc_dec_version = UBWC_1_0;
> > +             data->ubwc_enc_version = UBWC_1_0;
> > +     }
> > +
> > +     if (hw_rev == MDSS_HW_MSM8996 ||
> > +         hw_rev == MDSS_HW_MSM8998)
> > +             data->highest_bank_bit = 2;
> > +     else
> > +             data->highest_bank_bit = 1;
> > +
> > +     return data;
> > +}
> > +
> >   const struct msm_mdss_data *msm_mdss_get_mdss_data(struct device *dev)
> >   {
> >       struct msm_mdss *mdss;
> > @@ -222,6 +266,13 @@ const struct msm_mdss_data *msm_mdss_get_mdss_data(struct device *dev)
> >
> >       mdss = dev_get_drvdata(dev);
> >
> > +     /*
> > +      * We could not do it at the probe time, since hw revision register was
> > +      * not readable. Fill data structure now for the MDP5 platforms.
> > +      */
> > +     if (!mdss->mdss_data && mdss->is_mdp5)
> > +             mdss->mdss_data = msm_mdss_generate_mdp5_mdss_data(mdss);
> > +
> >       return mdss->mdss_data;
> >   }
> >
> >



-- 
With best wishes
Dmitry

Re: [PATCH v3 2/4] drm/msm/dpu: support binding to the mdp5 devices

[Abhinav Kumar]

On 2/7/2024 11:56 AM, Dmitry Baryshkov wrote:
> On Wed, 7 Feb 2024 at 20:48, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 1/5/2024 3:34 PM, Dmitry Baryshkov wrote:
>>> Existing MDP5 devices have slightly different bindings. The main
>>> register region is called `mdp_phys' instead of `mdp'. Also vbif
>>> register regions are a part of the parent, MDSS device. Add support for
>>> handling this binding differences.
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 98 ++++++++++++++++++++++++++-------
>>>    drivers/gpu/drm/msm/msm_drv.h           |  3 +
>>>    drivers/gpu/drm/msm/msm_io_utils.c      | 13 +++++
>>>    3 files changed, 93 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> index 723cc1d82143..aa9e0ad33ebb 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> @@ -1197,6 +1197,78 @@ static int dpu_kms_init(struct drm_device *ddev)
>>>        return 0;
>>>    }
>>>
>>> +static int dpu_kms_mmap_mdp5(struct dpu_kms *dpu_kms)
>>> +{
>>> +     struct platform_device *pdev = dpu_kms->pdev;
>>> +     struct platform_device *mdss_dev;
>>> +     int ret;
>>> +
>>> +     if (dpu_kms->pdev->dev.bus != &platform_bus_type)
>>> +             return -EINVAL;
>>> +
>>
>> !dev_is_platform() perhaps?
> 
> looks good
> 
>>
>> But I would like to understand this check a bit more. Can you pls
>> explain for which case this check was added?
> 
> It is necessary to be sure that we can perform to_platform_device() on
> the next line.
> 

Got it but on the next line we are using to_platform_device on dpu's 
parent device. So shouldnt the check also be dpu_kms->pdev->dev.parent.bus?

>>
>>> +     mdss_dev = to_platform_device(dpu_kms->pdev->dev.parent);
>>> +
>>> +     dpu_kms->mmio = msm_ioremap(pdev, "mdp_phys");
>>> +     if (IS_ERR(dpu_kms->mmio)) {
>>> +             ret = PTR_ERR(dpu_kms->mmio);
>>> +             DPU_ERROR("mdp register memory map failed: %d\n", ret);
>>> +             dpu_kms->mmio = NULL;
>>> +             return ret;
>>> +     }
>>> +     DRM_DEBUG("mapped dpu address space @%pK\n", dpu_kms->mmio);
>>> +
>>> +     dpu_kms->vbif[VBIF_RT] = msm_ioremap_mdss(mdss_dev,
>>> +                                               dpu_kms->pdev,
>>> +                                               "vbif_phys");
>>> +     if (IS_ERR(dpu_kms->vbif[VBIF_RT])) {
>>> +             ret = PTR_ERR(dpu_kms->vbif[VBIF_RT]);
>>> +             DPU_ERROR("vbif register memory map failed: %d\n", ret);
>>> +             dpu_kms->vbif[VBIF_RT] = NULL;
>>> +             return ret;
>>> +     }
>>> +
>>> +     dpu_kms->vbif[VBIF_NRT] = msm_ioremap_mdss(mdss_dev,
>>> +                                                dpu_kms->pdev,
>>> +                                                "vbif_nrt_phys");
>>
>> Do you think a "quiet" version would be better?
> 
> Yep, why not.
> 
>>
>>
>>> +     if (IS_ERR(dpu_kms->vbif[VBIF_NRT])) {
>>> +             dpu_kms->vbif[VBIF_NRT] = NULL;
>>> +             DPU_DEBUG("VBIF NRT is not defined");
>>> +     }
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static int dpu_kms_mmap_dpu(struct dpu_kms *dpu_kms)
>>> +{
>>> +     struct platform_device *pdev = dpu_kms->pdev;
>>> +     int ret;
>>> +
>>> +     dpu_kms->mmio = msm_ioremap(pdev, "mdp");
>>> +     if (IS_ERR(dpu_kms->mmio)) {
>>> +             ret = PTR_ERR(dpu_kms->mmio);
>>> +             DPU_ERROR("mdp register memory map failed: %d\n", ret);
>>> +             dpu_kms->mmio = NULL;
>>> +             return ret;
>>> +     }
>>> +     DRM_DEBUG("mapped dpu address space @%pK\n", dpu_kms->mmio);
>>> +
>>> +     dpu_kms->vbif[VBIF_RT] = msm_ioremap(pdev, "vbif");
>>> +     if (IS_ERR(dpu_kms->vbif[VBIF_RT])) {
>>> +             ret = PTR_ERR(dpu_kms->vbif[VBIF_RT]);
>>> +             DPU_ERROR("vbif register memory map failed: %d\n", ret);
>>> +             dpu_kms->vbif[VBIF_RT] = NULL;
>>> +             return ret;
>>> +     }
>>> +
>>> +     dpu_kms->vbif[VBIF_NRT] = msm_ioremap_quiet(pdev, "vbif_nrt");
>>> +     if (IS_ERR(dpu_kms->vbif[VBIF_NRT])) {
>>> +             dpu_kms->vbif[VBIF_NRT] = NULL;
>>> +             DPU_DEBUG("VBIF NRT is not defined");
>>> +     }
>>> +
>>> +     return 0;
>>> +}
>>> +
>>>    static int dpu_dev_probe(struct platform_device *pdev)
>>>    {
>>>        struct device *dev = &pdev->dev;
>>> @@ -1230,28 +1302,12 @@ static int dpu_dev_probe(struct platform_device *pdev)
>>>
>>>        dpu_kms->base.irq = irq;
>>>
>>> -     dpu_kms->mmio = msm_ioremap(pdev, "mdp");
>>> -     if (IS_ERR(dpu_kms->mmio)) {
>>> -             ret = PTR_ERR(dpu_kms->mmio);
>>> -             DPU_ERROR("mdp register memory map failed: %d\n", ret);
>>> -             dpu_kms->mmio = NULL;
>>> -             return ret;
>>> -     }
>>> -     DRM_DEBUG("mapped dpu address space @%pK\n", dpu_kms->mmio);
>>> -
>>> -     dpu_kms->vbif[VBIF_RT] = msm_ioremap(pdev, "vbif");
>>> -     if (IS_ERR(dpu_kms->vbif[VBIF_RT])) {
>>> -             ret = PTR_ERR(dpu_kms->vbif[VBIF_RT]);
>>> -             DPU_ERROR("vbif register memory map failed: %d\n", ret);
>>> -             dpu_kms->vbif[VBIF_RT] = NULL;
>>> +     if (of_device_is_compatible(dpu_kms->pdev->dev.of_node, "qcom,mdp5"))
>>> +             ret = dpu_kms_mmap_mdp5(dpu_kms);
>>> +     else
>>> +             ret = dpu_kms_mmap_dpu(dpu_kms);
>>> +     if (ret)
>>>                return ret;
>>> -     }
>>> -
>>> -     dpu_kms->vbif[VBIF_NRT] = msm_ioremap_quiet(pdev, "vbif_nrt");
>>> -     if (IS_ERR(dpu_kms->vbif[VBIF_NRT])) {
>>> -             dpu_kms->vbif[VBIF_NRT] = NULL;
>>> -             DPU_DEBUG("VBIF NRT is not defined");
>>> -     }
>>>
>>>        ret = dpu_kms_parse_data_bus_icc_path(dpu_kms);
>>>        if (ret)
>>> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
>>> index 16a7cbc0b7dd..01e783130054 100644
>>> --- a/drivers/gpu/drm/msm/msm_drv.h
>>> +++ b/drivers/gpu/drm/msm/msm_drv.h
>>> @@ -476,6 +476,9 @@ void __iomem *msm_ioremap(struct platform_device *pdev, const char *name);
>>>    void __iomem *msm_ioremap_size(struct platform_device *pdev, const char *name,
>>>                phys_addr_t *size);
>>>    void __iomem *msm_ioremap_quiet(struct platform_device *pdev, const char *name);
>>> +void __iomem *msm_ioremap_mdss(struct platform_device *mdss_pdev,
>>> +                            struct platform_device *dev,
>>> +                            const char *name);
>>>
>>>    struct icc_path *msm_icc_get(struct device *dev, const char *name);
>>>
>>> diff --git a/drivers/gpu/drm/msm/msm_io_utils.c b/drivers/gpu/drm/msm/msm_io_utils.c
>>> index 59d2788c4510..afedd61c3e28 100644
>>> --- a/drivers/gpu/drm/msm/msm_io_utils.c
>>> +++ b/drivers/gpu/drm/msm/msm_io_utils.c
>>> @@ -50,6 +50,19 @@ struct clk *msm_clk_get(struct platform_device *pdev, const char *name)
>>>        return clk;
>>>    }
>>>
>>> +void __iomem *msm_ioremap_mdss(struct platform_device *mdss_pdev,
>>> +                            struct platform_device *pdev,
>>> +                            const char *name)
>>> +{
>>> +     struct resource *res;
>>> +
>>> +     res = platform_get_resource_byname(mdss_pdev, IORESOURCE_MEM, name);
>>> +     if (!res)
>>> +             return ERR_PTR(-EINVAL);
>>> +
>>> +     return devm_ioremap_resource(&pdev->dev, res);
>>> +}
>>> +
>>>    static void __iomem *_msm_ioremap(struct platform_device *pdev, const char *name,
>>>                                  bool quiet, phys_addr_t *psize)
>>>    {
>>>
> 
> 
>