[RFC PATCH 0/4] drm/msm/mdss: rework UBWC setup

[RFC PATCH 0/4] drm/msm/mdss: rework UBWC setup

[Dmitry Baryshkov]

The commit 92bab9142456 ("drm/msm: less magic numbers in
msm_mdss_enable") reworked the static UBWC setup to replace magic
numbers with calulating written values from the SoC/device parameters.
This simplified adding new platforms.
However I did not estimate that the values would still be cryptic and
would be C&P-sted instead of being determined from the vendor DT. Some
of the platform (sc8180x) completely missed this setup step.

This series attempts to rework the static UBWC setup to be both
manageable and hopefully easier to write.

The series (being an RFC) depends on three pending platforms series
(sc8280xp, sm8350 and sm8450) with minor fixes applied on top of them
(e.g. to fix the hbb on sm8350).

Dmitry Baryshkov (4):
  drm/msm/mdss: convert UBWC setup to use match data
  drm/msm/mdss: correct the ubwc version for sm6115 platform
  drm/msm/mdss: add data for sc8180xp
  drm/msm/mdss: add the sdm845 data for completeness

 drivers/gpu/drm/msm/msm_mdss.c | 173 ++++++++++++++++++++-------------
 1 file changed, 107 insertions(+), 66 deletions(-)

-- 
2.35.1

[RFC PATCH 1/4] drm/msm/mdss: convert UBWC setup to use match data

[Dmitry Baryshkov]

To simplify adding new platforms and to make settings more obvious,
rewrite the UBWC setup to use the data structure to pass platform config
rather than just calling the functions direcly.

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

diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
index 92773e0a8fda..2219c1bd59a9 100644
--- a/drivers/gpu/drm/msm/msm_mdss.c
+++ b/drivers/gpu/drm/msm/msm_mdss.c
@@ -29,6 +29,14 @@
 
 #define MIN_IB_BW	400000000UL /* Min ib vote 400MB */
 
+struct msm_mdss_data {
+	u32 ubwc_version;
+	u32 ubwc_swizzle;
+	u32 ubwc_static;
+	u32 highest_bank_bit;
+	u32 macrotile_mode;
+};
+
 struct msm_mdss {
 	struct device *dev;
 
@@ -40,6 +48,7 @@ struct msm_mdss {
 		unsigned long enabled_mask;
 		struct irq_domain *domain;
 	} irq_controller;
+	const struct msm_mdss_data *mdss_data;
 	struct icc_path *path[2];
 	u32 num_paths;
 };
@@ -180,46 +189,40 @@ static int _msm_mdss_irq_domain_add(struct msm_mdss *msm_mdss)
 #define UBWC_3_0 0x30000000
 #define UBWC_4_0 0x40000000
 
-static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss,
-				       u32 ubwc_static)
+static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss)
 {
-	writel_relaxed(ubwc_static, msm_mdss->mmio + UBWC_STATIC);
+	const struct msm_mdss_data *data = msm_mdss->mdss_data;
+
+	writel_relaxed(data->ubwc_static, msm_mdss->mmio + UBWC_STATIC);
 }
 
-static void msm_mdss_setup_ubwc_dec_30(struct msm_mdss *msm_mdss,
-				       unsigned int ubwc_version,
-				       u32 ubwc_swizzle,
-				       u32 highest_bank_bit,
-				       u32 macrotile_mode)
+static void msm_mdss_setup_ubwc_dec_30(struct msm_mdss *msm_mdss)
 {
-	u32 value = (ubwc_swizzle & 0x1) |
-		    (highest_bank_bit & 0x3) << 4 |
-		    (macrotile_mode & 0x1) << 12;
+	const struct msm_mdss_data *data = msm_mdss->mdss_data;
+	u32 value = (data->ubwc_swizzle & 0x1) |
+		    (data->highest_bank_bit & 0x3) << 4 |
+		    (data->macrotile_mode & 0x1) << 12;
 
-	if (ubwc_version == UBWC_3_0)
+	if (data->ubwc_version == UBWC_3_0)
 		value |= BIT(10);
 
-	if (ubwc_version == UBWC_1_0)
+	if (data->ubwc_version == UBWC_1_0)
 		value |= BIT(8);
 
 	writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
 }
 
-static void msm_mdss_setup_ubwc_dec_40(struct msm_mdss *msm_mdss,
-				       unsigned int ubwc_version,
-				       u32 ubwc_swizzle,
-				       u32 ubwc_static,
-				       u32 highest_bank_bit,
-				       u32 macrotile_mode)
+static void msm_mdss_setup_ubwc_dec_40(struct msm_mdss *msm_mdss)
 {
-	u32 value = (ubwc_swizzle & 0x7) |
-		    (ubwc_static & 0x1) << 3 |
-		    (highest_bank_bit & 0x7) << 4 |
-		    (macrotile_mode & 0x1) << 12;
+	const struct msm_mdss_data *data = msm_mdss->mdss_data;
+	u32 value = (data->ubwc_swizzle & 0x7) |
+		    (data->ubwc_static & 0x1) << 3 |
+		    (data->highest_bank_bit & 0x7) << 4 |
+		    (data->macrotile_mode & 0x1) << 12;
 
 	writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
 
-	if (ubwc_version == UBWC_3_0) {
+	if (data->ubwc_version == UBWC_3_0) {
 		writel_relaxed(1, msm_mdss->mmio + UBWC_CTRL_2);
 		writel_relaxed(0, msm_mdss->mmio + UBWC_PREDICTION_MODE);
 	} else {
@@ -232,6 +235,7 @@ static int msm_mdss_enable(struct msm_mdss *msm_mdss)
 {
 	int ret;
 	u32 hw_rev;
+	u32 ubwc_dec_hw_version;
 
 	/*
 	 * Several components have AXI clocks that can only be turned on if
@@ -250,53 +254,36 @@ static int msm_mdss_enable(struct msm_mdss *msm_mdss)
 	 * HW_REV requires MDSS_MDP_CLK, which is not enabled by the mdss on
 	 * mdp5 hardware. Skip reading it for now.
 	 */
-	if (msm_mdss->is_mdp5)
+	if (msm_mdss->is_mdp5 || !msm_mdss->mdss_data)
 		return 0;
 
 	hw_rev = readl_relaxed(msm_mdss->mmio + HW_REV);
 	dev_dbg(msm_mdss->dev, "HW_REV: 0x%x\n", hw_rev);
+
+	ubwc_dec_hw_version = readl_relaxed(msm_mdss->mmio + UBWC_DEC_HW_VERSION);
 	dev_dbg(msm_mdss->dev, "UBWC_DEC_HW_VERSION: 0x%x\n",
-		readl_relaxed(msm_mdss->mmio + UBWC_DEC_HW_VERSION));
+		ubwc_dec_hw_version);
 
 	/*
 	 * ubwc config is part of the "mdss" region which is not accessible
 	 * from the rest of the driver. hardcode known configurations here
 	 *
 	 * Decoder version can be read from the UBWC_DEC_HW_VERSION reg,
-	 * UBWC_n and the rest of params comes from hw_catalog.
-	 * Unforunately this driver can not access hw catalog, so we have to
-	 * hardcode them here.
+	 * UBWC_n and the rest of params comes from hw data.
 	 */
-	switch (hw_rev) {
-	case DPU_HW_VER_500:
-	case DPU_HW_VER_501:
-		msm_mdss_setup_ubwc_dec_30(msm_mdss, UBWC_3_0, 0, 2, 0);
-		break;
-	case DPU_HW_VER_600:
-		/* TODO: highest_bank_bit = 2 for LP_DDR4 */
-		msm_mdss_setup_ubwc_dec_40(msm_mdss, UBWC_4_0, 6, 1, 3, 1);
-		break;
-	case DPU_HW_VER_620:
-		/* UBWC_2_0 */
-		msm_mdss_setup_ubwc_dec_20(msm_mdss, 0x1e);
+	switch (ubwc_dec_hw_version) {
+	case UBWC_2_0:
+		msm_mdss_setup_ubwc_dec_20(msm_mdss);
 		break;
-	case DPU_HW_VER_630:
-		/* UBWC_2_0 */
-		msm_mdss_setup_ubwc_dec_20(msm_mdss, 0x11f);
+	case UBWC_3_0:
+		msm_mdss_setup_ubwc_dec_30(msm_mdss);
 		break;
-	case DPU_HW_VER_700:
-		/* TODO: highest_bank_bit = 2 for LP_DDR4 */
-		msm_mdss_setup_ubwc_dec_40(msm_mdss, UBWC_4_0, 6, 1, 3, 1);
+	case UBWC_4_0:
+		msm_mdss_setup_ubwc_dec_40(msm_mdss);
 		break;
-	case DPU_HW_VER_720:
-		msm_mdss_setup_ubwc_dec_40(msm_mdss, UBWC_3_0, 6, 1, 1, 1);
-		break;
-	case DPU_HW_VER_800:
-		msm_mdss_setup_ubwc_dec_40(msm_mdss, UBWC_4_0, 6, 1, 2, 1);
-		break;
-	case DPU_HW_VER_810:
-		/* TODO: highest_bank_bit = 2 for LP_DDR4 */
-		msm_mdss_setup_ubwc_dec_40(msm_mdss, UBWC_4_0, 6, 1, 3, 1);
+	default:
+		dev_err(msm_mdss->dev, "Unuspported UBWC decoder version %x\n",
+			ubwc_dec_hw_version);
 		break;
 	}
 
@@ -487,6 +474,8 @@ static int mdss_probe(struct platform_device *pdev)
 	if (IS_ERR(mdss))
 		return PTR_ERR(mdss);
 
+	mdss->mdss_data = of_device_get_match_data(&pdev->dev);
+
 	platform_set_drvdata(pdev, mdss);
 
 	/*
@@ -516,20 +505,61 @@ static int mdss_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct msm_mdss_data sc7180_data = {
+	.ubwc_version = UBWC_2_0,
+	.ubwc_static = 0x1e,
+};
+
+static const struct msm_mdss_data sc7280_data = {
+	.ubwc_version = UBWC_3_0,
+	.ubwc_swizzle = 6,
+	.ubwc_static = 1,
+	.highest_bank_bit = 1,
+	.macrotile_mode = 1,
+};
+
+static const struct msm_mdss_data sc8280xp_data = {
+	.ubwc_version = UBWC_4_0,
+	.ubwc_swizzle = 6,
+	.ubwc_static = 1,
+	.highest_bank_bit = 2,
+	.macrotile_mode = 1,
+};
+
+static const struct msm_mdss_data sm8150_data = {
+	.ubwc_version = UBWC_3_0,
+	.highest_bank_bit = 2,
+};
+
+static const struct msm_mdss_data sm6115_data = {
+	.ubwc_version = UBWC_2_0,
+	.ubwc_swizzle = 7,
+	.ubwc_static = 0x11f,
+};
+
+static const struct msm_mdss_data sm8250_data = {
+	.ubwc_version = UBWC_4_0,
+	.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" },
 	{ .compatible = "qcom,qcm2290-mdss" },
 	{ .compatible = "qcom,sdm845-mdss" },
-	{ .compatible = "qcom,sc7180-mdss" },
-	{ .compatible = "qcom,sc7280-mdss" },
+	{ .compatible = "qcom,sc7180-mdss", .data = &sc7180_data },
+	{ .compatible = "qcom,sc7280-mdss", .data = &sc7280_data },
 	{ .compatible = "qcom,sc8180x-mdss" },
-	{ .compatible = "qcom,sc8280xp-mdss" },
-	{ .compatible = "qcom,sm6115-mdss" },
-	{ .compatible = "qcom,sm8150-mdss" },
-	{ .compatible = "qcom,sm8250-mdss" },
-	{ .compatible = "qcom,sm8350-mdss" },
-	{ .compatible = "qcom,sm8450-mdss" },
+	{ .compatible = "qcom,sc8280xp-mdss", .data = &sc8280xp_data },
+	{ .compatible = "qcom,sm6115-mdss", .data = &sm6115_data },
+	{ .compatible = "qcom,sm8150-mdss", .data = &sm8150_data },
+	{ .compatible = "qcom,sm8250-mdss", .data = &sm8250_data },
+	{ .compatible = "qcom,sm8350-mdss", .data = &sm8250_data },
+	{ .compatible = "qcom,sm8450-mdss", .data = &sm8250_data },
 	{}
 };
 MODULE_DEVICE_TABLE(of, mdss_dt_match);
-- 
2.35.1

Re: [RFC PATCH 1/4] drm/msm/mdss: convert UBWC setup to use match data

[Abhinav Kumar]

On 12/7/2022 4:08 PM, Dmitry Baryshkov wrote:
> To simplify adding new platforms and to make settings more obvious,
> rewrite the UBWC setup to use the data structure to pass platform config
> rather than just calling the functions direcly.

Why not use the catalog to store this information rather than using the 
platform device match data?

This seems more appropriate for the catalog.

> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/msm_mdss.c | 158 ++++++++++++++++++++-------------
>   1 file changed, 94 insertions(+), 64 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
> index 92773e0a8fda..2219c1bd59a9 100644
> --- a/drivers/gpu/drm/msm/msm_mdss.c
> +++ b/drivers/gpu/drm/msm/msm_mdss.c
> @@ -29,6 +29,14 @@
>   
>   #define MIN_IB_BW	400000000UL /* Min ib vote 400MB */
>   
> +struct msm_mdss_data {
> +	u32 ubwc_version;
> +	u32 ubwc_swizzle;
> +	u32 ubwc_static;
> +	u32 highest_bank_bit;
> +	u32 macrotile_mode;
> +};
> +
>   struct msm_mdss {
>   	struct device *dev;
>   
> @@ -40,6 +48,7 @@ struct msm_mdss {
>   		unsigned long enabled_mask;
>   		struct irq_domain *domain;
>   	} irq_controller;
> +	const struct msm_mdss_data *mdss_data;
>   	struct icc_path *path[2];
>   	u32 num_paths;
>   };
> @@ -180,46 +189,40 @@ static int _msm_mdss_irq_domain_add(struct msm_mdss *msm_mdss)
>   #define UBWC_3_0 0x30000000
>   #define UBWC_4_0 0x40000000
>   
> -static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss,
> -				       u32 ubwc_static)
> +static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss)
>   {
> -	writel_relaxed(ubwc_static, msm_mdss->mmio + UBWC_STATIC);
> +	const struct msm_mdss_data *data = msm_mdss->mdss_data;
> +
> +	writel_relaxed(data->ubwc_static, msm_mdss->mmio + UBWC_STATIC);
>   }
>   
> -static void msm_mdss_setup_ubwc_dec_30(struct msm_mdss *msm_mdss,
> -				       unsigned int ubwc_version,
> -				       u32 ubwc_swizzle,
> -				       u32 highest_bank_bit,
> -				       u32 macrotile_mode)
> +static void msm_mdss_setup_ubwc_dec_30(struct msm_mdss *msm_mdss)
>   {
> -	u32 value = (ubwc_swizzle & 0x1) |
> -		    (highest_bank_bit & 0x3) << 4 |
> -		    (macrotile_mode & 0x1) << 12;
> +	const struct msm_mdss_data *data = msm_mdss->mdss_data;
> +	u32 value = (data->ubwc_swizzle & 0x1) |
> +		    (data->highest_bank_bit & 0x3) << 4 |
> +		    (data->macrotile_mode & 0x1) << 12;
>   
> -	if (ubwc_version == UBWC_3_0)
> +	if (data->ubwc_version == UBWC_3_0)
>   		value |= BIT(10);
>   
> -	if (ubwc_version == UBWC_1_0)
> +	if (data->ubwc_version == UBWC_1_0)
>   		value |= BIT(8);
>   
>   	writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
>   }
>   
> -static void msm_mdss_setup_ubwc_dec_40(struct msm_mdss *msm_mdss,
> -				       unsigned int ubwc_version,
> -				       u32 ubwc_swizzle,
> -				       u32 ubwc_static,
> -				       u32 highest_bank_bit,
> -				       u32 macrotile_mode)
> +static void msm_mdss_setup_ubwc_dec_40(struct msm_mdss *msm_mdss)
>   {
> -	u32 value = (ubwc_swizzle & 0x7) |
> -		    (ubwc_static & 0x1) << 3 |
> -		    (highest_bank_bit & 0x7) << 4 |
> -		    (macrotile_mode & 0x1) << 12;
> +	const struct msm_mdss_data *data = msm_mdss->mdss_data;
> +	u32 value = (data->ubwc_swizzle & 0x7) |
> +		    (data->ubwc_static & 0x1) << 3 |
> +		    (data->highest_bank_bit & 0x7) << 4 |
> +		    (data->macrotile_mode & 0x1) << 12;
>   
>   	writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
>   
> -	if (ubwc_version == UBWC_3_0) {
> +	if (data->ubwc_version == UBWC_3_0) {
>   		writel_relaxed(1, msm_mdss->mmio + UBWC_CTRL_2);
>   		writel_relaxed(0, msm_mdss->mmio + UBWC_PREDICTION_MODE);
>   	} else {
> @@ -232,6 +235,7 @@ static int msm_mdss_enable(struct msm_mdss *msm_mdss)
>   {
>   	int ret;
>   	u32 hw_rev;
> +	u32 ubwc_dec_hw_version;
>   
>   	/*
>   	 * Several components have AXI clocks that can only be turned on if
> @@ -250,53 +254,36 @@ static int msm_mdss_enable(struct msm_mdss *msm_mdss)
>   	 * HW_REV requires MDSS_MDP_CLK, which is not enabled by the mdss on
>   	 * mdp5 hardware. Skip reading it for now.
>   	 */
> -	if (msm_mdss->is_mdp5)
> +	if (msm_mdss->is_mdp5 || !msm_mdss->mdss_data)
>   		return 0;
>   
>   	hw_rev = readl_relaxed(msm_mdss->mmio + HW_REV);
>   	dev_dbg(msm_mdss->dev, "HW_REV: 0x%x\n", hw_rev);
> +
> +	ubwc_dec_hw_version = readl_relaxed(msm_mdss->mmio + UBWC_DEC_HW_VERSION);
>   	dev_dbg(msm_mdss->dev, "UBWC_DEC_HW_VERSION: 0x%x\n",
> -		readl_relaxed(msm_mdss->mmio + UBWC_DEC_HW_VERSION));
> +		ubwc_dec_hw_version);
>   
>   	/*
>   	 * ubwc config is part of the "mdss" region which is not accessible
>   	 * from the rest of the driver. hardcode known configurations here
>   	 *
>   	 * Decoder version can be read from the UBWC_DEC_HW_VERSION reg,
> -	 * UBWC_n and the rest of params comes from hw_catalog.
> -	 * Unforunately this driver can not access hw catalog, so we have to
> -	 * hardcode them here.
> +	 * UBWC_n and the rest of params comes from hw data.
>   	 */
> -	switch (hw_rev) {
> -	case DPU_HW_VER_500:
> -	case DPU_HW_VER_501:
> -		msm_mdss_setup_ubwc_dec_30(msm_mdss, UBWC_3_0, 0, 2, 0);
> -		break;
> -	case DPU_HW_VER_600:
> -		/* TODO: highest_bank_bit = 2 for LP_DDR4 */
> -		msm_mdss_setup_ubwc_dec_40(msm_mdss, UBWC_4_0, 6, 1, 3, 1);
> -		break;
> -	case DPU_HW_VER_620:
> -		/* UBWC_2_0 */
> -		msm_mdss_setup_ubwc_dec_20(msm_mdss, 0x1e);
> +	switch (ubwc_dec_hw_version) {
> +	case UBWC_2_0:
> +		msm_mdss_setup_ubwc_dec_20(msm_mdss);
>   		break;
> -	case DPU_HW_VER_630:
> -		/* UBWC_2_0 */
> -		msm_mdss_setup_ubwc_dec_20(msm_mdss, 0x11f);
> +	case UBWC_3_0:
> +		msm_mdss_setup_ubwc_dec_30(msm_mdss);
>   		break;
> -	case DPU_HW_VER_700:
> -		/* TODO: highest_bank_bit = 2 for LP_DDR4 */
> -		msm_mdss_setup_ubwc_dec_40(msm_mdss, UBWC_4_0, 6, 1, 3, 1);
> +	case UBWC_4_0:
> +		msm_mdss_setup_ubwc_dec_40(msm_mdss);
>   		break;
> -	case DPU_HW_VER_720:
> -		msm_mdss_setup_ubwc_dec_40(msm_mdss, UBWC_3_0, 6, 1, 1, 1);
> -		break;
> -	case DPU_HW_VER_800:
> -		msm_mdss_setup_ubwc_dec_40(msm_mdss, UBWC_4_0, 6, 1, 2, 1);
> -		break;
> -	case DPU_HW_VER_810:
> -		/* TODO: highest_bank_bit = 2 for LP_DDR4 */
> -		msm_mdss_setup_ubwc_dec_40(msm_mdss, UBWC_4_0, 6, 1, 3, 1);
> +	default:
> +		dev_err(msm_mdss->dev, "Unuspported UBWC decoder version %x\n",
> +			ubwc_dec_hw_version);
>   		break;
>   	}
>   
> @@ -487,6 +474,8 @@ static int mdss_probe(struct platform_device *pdev)
>   	if (IS_ERR(mdss))
>   		return PTR_ERR(mdss);
>   
> +	mdss->mdss_data = of_device_get_match_data(&pdev->dev);
> +
>   	platform_set_drvdata(pdev, mdss);
>   
>   	/*
> @@ -516,20 +505,61 @@ static int mdss_remove(struct platform_device *pdev)
>   	return 0;
>   }
>   
> +static const struct msm_mdss_data sc7180_data = {
> +	.ubwc_version = UBWC_2_0,
> +	.ubwc_static = 0x1e,
> +};
> +
> +static const struct msm_mdss_data sc7280_data = {
> +	.ubwc_version = UBWC_3_0,
> +	.ubwc_swizzle = 6,
> +	.ubwc_static = 1,
> +	.highest_bank_bit = 1,
> +	.macrotile_mode = 1,
> +};
> +
> +static const struct msm_mdss_data sc8280xp_data = {
> +	.ubwc_version = UBWC_4_0,
> +	.ubwc_swizzle = 6,
> +	.ubwc_static = 1,
> +	.highest_bank_bit = 2,
> +	.macrotile_mode = 1,
> +};
> +
> +static const struct msm_mdss_data sm8150_data = {
> +	.ubwc_version = UBWC_3_0,
> +	.highest_bank_bit = 2,
> +};
> +
> +static const struct msm_mdss_data sm6115_data = {
> +	.ubwc_version = UBWC_2_0,
> +	.ubwc_swizzle = 7,
> +	.ubwc_static = 0x11f,
> +};
> +
> +static const struct msm_mdss_data sm8250_data = {
> +	.ubwc_version = UBWC_4_0,
> +	.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" },
>   	{ .compatible = "qcom,qcm2290-mdss" },
>   	{ .compatible = "qcom,sdm845-mdss" },
> -	{ .compatible = "qcom,sc7180-mdss" },
> -	{ .compatible = "qcom,sc7280-mdss" },
> +	{ .compatible = "qcom,sc7180-mdss", .data = &sc7180_data },
> +	{ .compatible = "qcom,sc7280-mdss", .data = &sc7280_data },
>   	{ .compatible = "qcom,sc8180x-mdss" },
> -	{ .compatible = "qcom,sc8280xp-mdss" },
> -	{ .compatible = "qcom,sm6115-mdss" },
> -	{ .compatible = "qcom,sm8150-mdss" },
> -	{ .compatible = "qcom,sm8250-mdss" },
> -	{ .compatible = "qcom,sm8350-mdss" },
> -	{ .compatible = "qcom,sm8450-mdss" },
> +	{ .compatible = "qcom,sc8280xp-mdss", .data = &sc8280xp_data },
> +	{ .compatible = "qcom,sm6115-mdss", .data = &sm6115_data },
> +	{ .compatible = "qcom,sm8150-mdss", .data = &sm8150_data },
> +	{ .compatible = "qcom,sm8250-mdss", .data = &sm8250_data },
> +	{ .compatible = "qcom,sm8350-mdss", .data = &sm8250_data },
> +	{ .compatible = "qcom,sm8450-mdss", .data = &sm8250_data },
>   	{}
>   };
>   MODULE_DEVICE_TABLE(of, mdss_dt_match);

Re: [RFC PATCH 1/4] drm/msm/mdss: convert UBWC setup to use match data

[Dmitry Baryshkov]

On 09/01/2023 21:53, Abhinav Kumar wrote:
> 
> 
> On 12/7/2022 4:08 PM, Dmitry Baryshkov wrote:
>> To simplify adding new platforms and to make settings more obvious,
>> rewrite the UBWC setup to use the data structure to pass platform config
>> rather than just calling the functions direcly.
> 
> Why not use the catalog to store this information rather than using the 
> platform device match data?
> 
> This seems more appropriate for the catalog.

Which catalog?

If you are talking about the DPU hw catalog, it's not possible. DPU and 
MDSS are two distinct drivers even if they are built into the same module.

And if you are talking about adding mdss_catalog, I'd abstain from that 
idea. It is too easy to update one piece and forget the other one. Using 
match data is what other drivers are using (and it ensures that each new 
supported device gets its correct match data).

> 
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/gpu/drm/msm/msm_mdss.c | 158 ++++++++++++++++++++-------------
>>   1 file changed, 94 insertions(+), 64 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/msm_mdss.c 
>> b/drivers/gpu/drm/msm/msm_mdss.c
>> index 92773e0a8fda..2219c1bd59a9 100644
>> --- a/drivers/gpu/drm/msm/msm_mdss.c
>> +++ b/drivers/gpu/drm/msm/msm_mdss.c
>> @@ -29,6 +29,14 @@
>>   #define MIN_IB_BW    400000000UL /* Min ib vote 400MB */
>> +struct msm_mdss_data {
>> +    u32 ubwc_version;
>> +    u32 ubwc_swizzle;
>> +    u32 ubwc_static;
>> +    u32 highest_bank_bit;
>> +    u32 macrotile_mode;
>> +};
>> +
>>   struct msm_mdss {
>>       struct device *dev;
>> @@ -40,6 +48,7 @@ struct msm_mdss {
>>           unsigned long enabled_mask;
>>           struct irq_domain *domain;
>>       } irq_controller;
>> +    const struct msm_mdss_data *mdss_data;
>>       struct icc_path *path[2];
>>       u32 num_paths;
>>   };
>> @@ -180,46 +189,40 @@ static int _msm_mdss_irq_domain_add(struct 
>> msm_mdss *msm_mdss)
>>   #define UBWC_3_0 0x30000000
>>   #define UBWC_4_0 0x40000000
>> -static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss,
>> -                       u32 ubwc_static)
>> +static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss)
>>   {
>> -    writel_relaxed(ubwc_static, msm_mdss->mmio + UBWC_STATIC);
>> +    const struct msm_mdss_data *data = msm_mdss->mdss_data;
>> +
>> +    writel_relaxed(data->ubwc_static, msm_mdss->mmio + UBWC_STATIC);
>>   }
>> -static void msm_mdss_setup_ubwc_dec_30(struct msm_mdss *msm_mdss,
>> -                       unsigned int ubwc_version,
>> -                       u32 ubwc_swizzle,
>> -                       u32 highest_bank_bit,
>> -                       u32 macrotile_mode)
>> +static void msm_mdss_setup_ubwc_dec_30(struct msm_mdss *msm_mdss)
>>   {
>> -    u32 value = (ubwc_swizzle & 0x1) |
>> -            (highest_bank_bit & 0x3) << 4 |
>> -            (macrotile_mode & 0x1) << 12;
>> +    const struct msm_mdss_data *data = msm_mdss->mdss_data;
>> +    u32 value = (data->ubwc_swizzle & 0x1) |
>> +            (data->highest_bank_bit & 0x3) << 4 |
>> +            (data->macrotile_mode & 0x1) << 12;
>> -    if (ubwc_version == UBWC_3_0)
>> +    if (data->ubwc_version == UBWC_3_0)
>>           value |= BIT(10);
>> -    if (ubwc_version == UBWC_1_0)
>> +    if (data->ubwc_version == UBWC_1_0)
>>           value |= BIT(8);
>>       writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
>>   }
>> -static void msm_mdss_setup_ubwc_dec_40(struct msm_mdss *msm_mdss,
>> -                       unsigned int ubwc_version,
>> -                       u32 ubwc_swizzle,
>> -                       u32 ubwc_static,
>> -                       u32 highest_bank_bit,
>> -                       u32 macrotile_mode)
>> +static void msm_mdss_setup_ubwc_dec_40(struct msm_mdss *msm_mdss)
>>   {
>> -    u32 value = (ubwc_swizzle & 0x7) |
>> -            (ubwc_static & 0x1) << 3 |
>> -            (highest_bank_bit & 0x7) << 4 |
>> -            (macrotile_mode & 0x1) << 12;
>> +    const struct msm_mdss_data *data = msm_mdss->mdss_data;
>> +    u32 value = (data->ubwc_swizzle & 0x7) |
>> +            (data->ubwc_static & 0x1) << 3 |
>> +            (data->highest_bank_bit & 0x7) << 4 |
>> +            (data->macrotile_mode & 0x1) << 12;
>>       writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
>> -    if (ubwc_version == UBWC_3_0) {
>> +    if (data->ubwc_version == UBWC_3_0) {
>>           writel_relaxed(1, msm_mdss->mmio + UBWC_CTRL_2);
>>           writel_relaxed(0, msm_mdss->mmio + UBWC_PREDICTION_MODE);
>>       } else {
>> @@ -232,6 +235,7 @@ static int msm_mdss_enable(struct msm_mdss *msm_mdss)
>>   {
>>       int ret;
>>       u32 hw_rev;
>> +    u32 ubwc_dec_hw_version;
>>       /*
>>        * Several components have AXI clocks that can only be turned on if
>> @@ -250,53 +254,36 @@ static int msm_mdss_enable(struct msm_mdss 
>> *msm_mdss)
>>        * HW_REV requires MDSS_MDP_CLK, which is not enabled by the 
>> mdss on
>>        * mdp5 hardware. Skip reading it for now.
>>        */
>> -    if (msm_mdss->is_mdp5)
>> +    if (msm_mdss->is_mdp5 || !msm_mdss->mdss_data)
>>           return 0;
>>       hw_rev = readl_relaxed(msm_mdss->mmio + HW_REV);
>>       dev_dbg(msm_mdss->dev, "HW_REV: 0x%x\n", hw_rev);
>> +
>> +    ubwc_dec_hw_version = readl_relaxed(msm_mdss->mmio + 
>> UBWC_DEC_HW_VERSION);
>>       dev_dbg(msm_mdss->dev, "UBWC_DEC_HW_VERSION: 0x%x\n",
>> -        readl_relaxed(msm_mdss->mmio + UBWC_DEC_HW_VERSION));
>> +        ubwc_dec_hw_version);
>>       /*
>>        * ubwc config is part of the "mdss" region which is not accessible
>>        * from the rest of the driver. hardcode known configurations here
>>        *
>>        * Decoder version can be read from the UBWC_DEC_HW_VERSION reg,
>> -     * UBWC_n and the rest of params comes from hw_catalog.
>> -     * Unforunately this driver can not access hw catalog, so we have to
>> -     * hardcode them here.
>> +     * UBWC_n and the rest of params comes from hw data.
>>        */
>> -    switch (hw_rev) {
>> -    case DPU_HW_VER_500:
>> -    case DPU_HW_VER_501:
>> -        msm_mdss_setup_ubwc_dec_30(msm_mdss, UBWC_3_0, 0, 2, 0);
>> -        break;
>> -    case DPU_HW_VER_600:
>> -        /* TODO: highest_bank_bit = 2 for LP_DDR4 */
>> -        msm_mdss_setup_ubwc_dec_40(msm_mdss, UBWC_4_0, 6, 1, 3, 1);
>> -        break;
>> -    case DPU_HW_VER_620:
>> -        /* UBWC_2_0 */
>> -        msm_mdss_setup_ubwc_dec_20(msm_mdss, 0x1e);
>> +    switch (ubwc_dec_hw_version) {
>> +    case UBWC_2_0:
>> +        msm_mdss_setup_ubwc_dec_20(msm_mdss);
>>           break;
>> -    case DPU_HW_VER_630:
>> -        /* UBWC_2_0 */
>> -        msm_mdss_setup_ubwc_dec_20(msm_mdss, 0x11f);
>> +    case UBWC_3_0:
>> +        msm_mdss_setup_ubwc_dec_30(msm_mdss);
>>           break;
>> -    case DPU_HW_VER_700:
>> -        /* TODO: highest_bank_bit = 2 for LP_DDR4 */
>> -        msm_mdss_setup_ubwc_dec_40(msm_mdss, UBWC_4_0, 6, 1, 3, 1);
>> +    case UBWC_4_0:
>> +        msm_mdss_setup_ubwc_dec_40(msm_mdss);
>>           break;
>> -    case DPU_HW_VER_720:
>> -        msm_mdss_setup_ubwc_dec_40(msm_mdss, UBWC_3_0, 6, 1, 1, 1);
>> -        break;
>> -    case DPU_HW_VER_800:
>> -        msm_mdss_setup_ubwc_dec_40(msm_mdss, UBWC_4_0, 6, 1, 2, 1);
>> -        break;
>> -    case DPU_HW_VER_810:
>> -        /* TODO: highest_bank_bit = 2 for LP_DDR4 */
>> -        msm_mdss_setup_ubwc_dec_40(msm_mdss, UBWC_4_0, 6, 1, 3, 1);
>> +    default:
>> +        dev_err(msm_mdss->dev, "Unuspported UBWC decoder version %x\n",
>> +            ubwc_dec_hw_version);
>>           break;
>>       }
>> @@ -487,6 +474,8 @@ static int mdss_probe(struct platform_device *pdev)
>>       if (IS_ERR(mdss))
>>           return PTR_ERR(mdss);
>> +    mdss->mdss_data = of_device_get_match_data(&pdev->dev);
>> +
>>       platform_set_drvdata(pdev, mdss);
>>       /*
>> @@ -516,20 +505,61 @@ static int mdss_remove(struct platform_device 
>> *pdev)
>>       return 0;
>>   }
>> +static const struct msm_mdss_data sc7180_data = {
>> +    .ubwc_version = UBWC_2_0,
>> +    .ubwc_static = 0x1e,
>> +};
>> +
>> +static const struct msm_mdss_data sc7280_data = {
>> +    .ubwc_version = UBWC_3_0,
>> +    .ubwc_swizzle = 6,
>> +    .ubwc_static = 1,
>> +    .highest_bank_bit = 1,
>> +    .macrotile_mode = 1,
>> +};
>> +
>> +static const struct msm_mdss_data sc8280xp_data = {
>> +    .ubwc_version = UBWC_4_0,
>> +    .ubwc_swizzle = 6,
>> +    .ubwc_static = 1,
>> +    .highest_bank_bit = 2,
>> +    .macrotile_mode = 1,
>> +};
>> +
>> +static const struct msm_mdss_data sm8150_data = {
>> +    .ubwc_version = UBWC_3_0,
>> +    .highest_bank_bit = 2,
>> +};
>> +
>> +static const struct msm_mdss_data sm6115_data = {
>> +    .ubwc_version = UBWC_2_0,
>> +    .ubwc_swizzle = 7,
>> +    .ubwc_static = 0x11f,
>> +};
>> +
>> +static const struct msm_mdss_data sm8250_data = {
>> +    .ubwc_version = UBWC_4_0,
>> +    .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" },
>>       { .compatible = "qcom,qcm2290-mdss" },
>>       { .compatible = "qcom,sdm845-mdss" },
>> -    { .compatible = "qcom,sc7180-mdss" },
>> -    { .compatible = "qcom,sc7280-mdss" },
>> +    { .compatible = "qcom,sc7180-mdss", .data = &sc7180_data },
>> +    { .compatible = "qcom,sc7280-mdss", .data = &sc7280_data },
>>       { .compatible = "qcom,sc8180x-mdss" },
>> -    { .compatible = "qcom,sc8280xp-mdss" },
>> -    { .compatible = "qcom,sm6115-mdss" },
>> -    { .compatible = "qcom,sm8150-mdss" },
>> -    { .compatible = "qcom,sm8250-mdss" },
>> -    { .compatible = "qcom,sm8350-mdss" },
>> -    { .compatible = "qcom,sm8450-mdss" },
>> +    { .compatible = "qcom,sc8280xp-mdss", .data = &sc8280xp_data },
>> +    { .compatible = "qcom,sm6115-mdss", .data = &sm6115_data },
>> +    { .compatible = "qcom,sm8150-mdss", .data = &sm8150_data },
>> +    { .compatible = "qcom,sm8250-mdss", .data = &sm8250_data },
>> +    { .compatible = "qcom,sm8350-mdss", .data = &sm8250_data },
>> +    { .compatible = "qcom,sm8450-mdss", .data = &sm8250_data },
>>       {}
>>   };
>>   MODULE_DEVICE_TABLE(of, mdss_dt_match);

-- 
With best wishes
Dmitry

Re: [RFC PATCH 1/4] drm/msm/mdss: convert UBWC setup to use match data

[Abhinav Kumar]

On 1/9/2023 12:17 PM, Dmitry Baryshkov wrote:
> On 09/01/2023 21:53, Abhinav Kumar wrote:
>>
>>
>> On 12/7/2022 4:08 PM, Dmitry Baryshkov wrote:
>>> To simplify adding new platforms and to make settings more obvious,
>>> rewrite the UBWC setup to use the data structure to pass platform config
>>> rather than just calling the functions direcly.
>>
>> Why not use the catalog to store this information rather than using 
>> the platform device match data?
>>
>> This seems more appropriate for the catalog.
> 
> Which catalog?
> 
> If you are talking about the DPU hw catalog, it's not possible. DPU and 
> MDSS are two distinct drivers even if they are built into the same module.
> 
> And if you are talking about adding mdss_catalog, I'd abstain from that 
> idea. It is too easy to update one piece and forget the other one. Using 
> match data is what other drivers are using (and it ensures that each new 
> supported device gets its correct match data).
> 

Yes, I was referring to the DPU catalog.

But now I recall the mess because of the UBWC register being part of 
mmio base which the DPU doesnt map.

I do think that the platform match data is a bit of an overkill just to 
store the UBWC values but the msm_mdss driver today doesnt program 
anything else today so lets go with this.

But ... some comments below.

>>
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>   drivers/gpu/drm/msm/msm_mdss.c | 158 ++++++++++++++++++++-------------
>>>   1 file changed, 94 insertions(+), 64 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/msm_mdss.c 
>>> b/drivers/gpu/drm/msm/msm_mdss.c
>>> index 92773e0a8fda..2219c1bd59a9 100644
>>> --- a/drivers/gpu/drm/msm/msm_mdss.c
>>> +++ b/drivers/gpu/drm/msm/msm_mdss.c
>>> @@ -29,6 +29,14 @@
>>>   #define MIN_IB_BW    400000000UL /* Min ib vote 400MB */
>>> +struct msm_mdss_data {
>>> +    u32 ubwc_version;
>>> +    u32 ubwc_swizzle;
>>> +    u32 ubwc_static;
>>> +    u32 highest_bank_bit;
>>> +    u32 macrotile_mode;
>>> +};
>>> +
>>>   struct msm_mdss {
>>>       struct device *dev;
>>> @@ -40,6 +48,7 @@ struct msm_mdss {
>>>           unsigned long enabled_mask;
>>>           struct irq_domain *domain;
>>>       } irq_controller;
>>> +    const struct msm_mdss_data *mdss_data;
>>>       struct icc_path *path[2];
>>>       u32 num_paths;
>>>   };
>>> @@ -180,46 +189,40 @@ static int _msm_mdss_irq_domain_add(struct 
>>> msm_mdss *msm_mdss)
>>>   #define UBWC_3_0 0x30000000
>>>   #define UBWC_4_0 0x40000000
>>> -static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss,
>>> -                       u32 ubwc_static)
>>> +static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss)
>>>   {
>>> -    writel_relaxed(ubwc_static, msm_mdss->mmio + UBWC_STATIC);
>>> +    const struct msm_mdss_data *data = msm_mdss->mdss_data;
>>> +
>>> +    writel_relaxed(data->ubwc_static, msm_mdss->mmio + UBWC_STATIC);
>>>   }
>>> -static void msm_mdss_setup_ubwc_dec_30(struct msm_mdss *msm_mdss,
>>> -                       unsigned int ubwc_version,
>>> -                       u32 ubwc_swizzle,
>>> -                       u32 highest_bank_bit,
>>> -                       u32 macrotile_mode)
>>> +static void msm_mdss_setup_ubwc_dec_30(struct msm_mdss *msm_mdss)
>>>   {
>>> -    u32 value = (ubwc_swizzle & 0x1) |
>>> -            (highest_bank_bit & 0x3) << 4 |
>>> -            (macrotile_mode & 0x1) << 12;
>>> +    const struct msm_mdss_data *data = msm_mdss->mdss_data;
>>> +    u32 value = (data->ubwc_swizzle & 0x1) |
>>> +            (data->highest_bank_bit & 0x3) << 4 |
>>> +            (data->macrotile_mode & 0x1) << 12;
>>> -    if (ubwc_version == UBWC_3_0)
>>> +    if (data->ubwc_version == UBWC_3_0)
>>>           value |= BIT(10);
>>> -    if (ubwc_version == UBWC_1_0)
>>> +    if (data->ubwc_version == UBWC_1_0)
>>>           value |= BIT(8);
>>>       writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
>>>   }
>>> -static void msm_mdss_setup_ubwc_dec_40(struct msm_mdss *msm_mdss,
>>> -                       unsigned int ubwc_version,
>>> -                       u32 ubwc_swizzle,
>>> -                       u32 ubwc_static,
>>> -                       u32 highest_bank_bit,
>>> -                       u32 macrotile_mode)
>>> +static void msm_mdss_setup_ubwc_dec_40(struct msm_mdss *msm_mdss)
>>>   {
>>> -    u32 value = (ubwc_swizzle & 0x7) |
>>> -            (ubwc_static & 0x1) << 3 |
>>> -            (highest_bank_bit & 0x7) << 4 |
>>> -            (macrotile_mode & 0x1) << 12;
>>> +    const struct msm_mdss_data *data = msm_mdss->mdss_data;
>>> +    u32 value = (data->ubwc_swizzle & 0x7) |
>>> +            (data->ubwc_static & 0x1) << 3 |
>>> +            (data->highest_bank_bit & 0x7) << 4 |
>>> +            (data->macrotile_mode & 0x1) << 12;
>>>       writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
>>> -    if (ubwc_version == UBWC_3_0) {
>>> +    if (data->ubwc_version == UBWC_3_0) {
>>>           writel_relaxed(1, msm_mdss->mmio + UBWC_CTRL_2);
>>>           writel_relaxed(0, msm_mdss->mmio + UBWC_PREDICTION_MODE);
>>>       } else {
>>> @@ -232,6 +235,7 @@ static int msm_mdss_enable(struct msm_mdss 
>>> *msm_mdss)
>>>   {
>>>       int ret;
>>>       u32 hw_rev;
>>> +    u32 ubwc_dec_hw_version;
>>>       /*
>>>        * Several components have AXI clocks that can only be turned 
>>> on if
>>> @@ -250,53 +254,36 @@ static int msm_mdss_enable(struct msm_mdss 
>>> *msm_mdss)
>>>        * HW_REV requires MDSS_MDP_CLK, which is not enabled by the 
>>> mdss on
>>>        * mdp5 hardware. Skip reading it for now.
>>>        */
>>> -    if (msm_mdss->is_mdp5)
>>> +    if (msm_mdss->is_mdp5 || !msm_mdss->mdss_data)
>>>           return 0;
>>>       hw_rev = readl_relaxed(msm_mdss->mmio + HW_REV);

hw_rev is not used anymore now so why not just drop that reg read 
altogether.

>>>       dev_dbg(msm_mdss->dev, "HW_REV: 0x%x\n", hw_rev);
>>> +
>>> +    ubwc_dec_hw_version = readl_relaxed(msm_mdss->mmio + 
>>> UBWC_DEC_HW_VERSION);

If we are going to tie UBWC version to the HW compatible match, then 
even this register read can be skipped and instead you can add 
ubwc_dec_hw_version to your match data struct and skip this read as well.

That way we get rid of all register reads in this path which have 
continuously bugged us with crashes.

>>>       dev_dbg(msm_mdss->dev, "UBWC_DEC_HW_VERSION: 0x%x\n",
>>> -        readl_relaxed(msm_mdss->mmio + UBWC_DEC_HW_VERSION));
>>> +        ubwc_dec_hw_version);
>>>       /*
>>>        * ubwc config is part of the "mdss" region which is not 
>>> accessible
>>>        * from the rest of the driver. hardcode known configurations here
>>>        *
>>>        * Decoder version can be read from the UBWC_DEC_HW_VERSION reg,
>>> -     * UBWC_n and the rest of params comes from hw_catalog.
>>> -     * Unforunately this driver can not access hw catalog, so we 
>>> have to
>>> -     * hardcode them here.
>>> +     * UBWC_n and the rest of params comes from hw data.
>>>        */
>>> -    switch (hw_rev) {
>>> -    case DPU_HW_VER_500:
>>> -    case DPU_HW_VER_501:
>>> -        msm_mdss_setup_ubwc_dec_30(msm_mdss, UBWC_3_0, 0, 2, 0);
>>> -        break;
>>> -    case DPU_HW_VER_600:
>>> -        /* TODO: highest_bank_bit = 2 for LP_DDR4 */
>>> -        msm_mdss_setup_ubwc_dec_40(msm_mdss, UBWC_4_0, 6, 1, 3, 1);
>>> -        break;
>>> -    case DPU_HW_VER_620:
>>> -        /* UBWC_2_0 */
>>> -        msm_mdss_setup_ubwc_dec_20(msm_mdss, 0x1e);
>>> +    switch (ubwc_dec_hw_version) {
>>> +    case UBWC_2_0:
>>> +        msm_mdss_setup_ubwc_dec_20(msm_mdss);
>>>           break;
>>> -    case DPU_HW_VER_630:
>>> -        /* UBWC_2_0 */
>>> -        msm_mdss_setup_ubwc_dec_20(msm_mdss, 0x11f);
>>> +    case UBWC_3_0:
>>> +        msm_mdss_setup_ubwc_dec_30(msm_mdss);
>>>           break;
>>> -    case DPU_HW_VER_700:
>>> -        /* TODO: highest_bank_bit = 2 for LP_DDR4 */
>>> -        msm_mdss_setup_ubwc_dec_40(msm_mdss, UBWC_4_0, 6, 1, 3, 1);
>>> +    case UBWC_4_0:
>>> +        msm_mdss_setup_ubwc_dec_40(msm_mdss);
>>>           break;
>>> -    case DPU_HW_VER_720:
>>> -        msm_mdss_setup_ubwc_dec_40(msm_mdss, UBWC_3_0, 6, 1, 1, 1);
>>> -        break;
>>> -    case DPU_HW_VER_800:
>>> -        msm_mdss_setup_ubwc_dec_40(msm_mdss, UBWC_4_0, 6, 1, 2, 1);
>>> -        break;
>>> -    case DPU_HW_VER_810:
>>> -        /* TODO: highest_bank_bit = 2 for LP_DDR4 */
>>> -        msm_mdss_setup_ubwc_dec_40(msm_mdss, UBWC_4_0, 6, 1, 3, 1);
>>> +    default:
>>> +        dev_err(msm_mdss->dev, "Unuspported UBWC decoder version %x\n",
>>> +            ubwc_dec_hw_version);
>>>           break;
>>>       }
>>> @@ -487,6 +474,8 @@ static int mdss_probe(struct platform_device *pdev)
>>>       if (IS_ERR(mdss))
>>>           return PTR_ERR(mdss);
>>> +    mdss->mdss_data = of_device_get_match_data(&pdev->dev);
>>> +
>>>       platform_set_drvdata(pdev, mdss);
>>>       /*
>>> @@ -516,20 +505,61 @@ static int mdss_remove(struct platform_device 
>>> *pdev)
>>>       return 0;
>>>   }
>>> +static const struct msm_mdss_data sc7180_data = {
>>> +    .ubwc_version = UBWC_2_0,
>>> +    .ubwc_static = 0x1e,
>>> +};
>>> +
>>> +static const struct msm_mdss_data sc7280_data = {
>>> +    .ubwc_version = UBWC_3_0,
>>> +    .ubwc_swizzle = 6,
>>> +    .ubwc_static = 1,
>>> +    .highest_bank_bit = 1,
>>> +    .macrotile_mode = 1,
>>> +};
>>> +
>>> +static const struct msm_mdss_data sc8280xp_data = {
>>> +    .ubwc_version = UBWC_4_0,
>>> +    .ubwc_swizzle = 6,
>>> +    .ubwc_static = 1,
>>> +    .highest_bank_bit = 2,
>>> +    .macrotile_mode = 1,
>>> +};
>>> +
>>> +static const struct msm_mdss_data sm8150_data = {
>>> +    .ubwc_version = UBWC_3_0,
>>> +    .highest_bank_bit = 2,
>>> +};
>>> +
>>> +static const struct msm_mdss_data sm6115_data = {
>>> +    .ubwc_version = UBWC_2_0,
>>> +    .ubwc_swizzle = 7,
>>> +    .ubwc_static = 0x11f,
>>> +};
>>> +
>>> +static const struct msm_mdss_data sm8250_data = {
>>> +    .ubwc_version = UBWC_4_0,
>>> +    .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" },
>>>       { .compatible = "qcom,qcm2290-mdss" },
>>>       { .compatible = "qcom,sdm845-mdss" },
>>> -    { .compatible = "qcom,sc7180-mdss" },
>>> -    { .compatible = "qcom,sc7280-mdss" },
>>> +    { .compatible = "qcom,sc7180-mdss", .data = &sc7180_data },
>>> +    { .compatible = "qcom,sc7280-mdss", .data = &sc7280_data },
>>>       { .compatible = "qcom,sc8180x-mdss" },
>>> -    { .compatible = "qcom,sc8280xp-mdss" },
>>> -    { .compatible = "qcom,sm6115-mdss" },
>>> -    { .compatible = "qcom,sm8150-mdss" },
>>> -    { .compatible = "qcom,sm8250-mdss" },
>>> -    { .compatible = "qcom,sm8350-mdss" },
>>> -    { .compatible = "qcom,sm8450-mdss" },
>>> +    { .compatible = "qcom,sc8280xp-mdss", .data = &sc8280xp_data },
>>> +    { .compatible = "qcom,sm6115-mdss", .data = &sm6115_data },
>>> +    { .compatible = "qcom,sm8150-mdss", .data = &sm8150_data },
>>> +    { .compatible = "qcom,sm8250-mdss", .data = &sm8250_data },
>>> +    { .compatible = "qcom,sm8350-mdss", .data = &sm8250_data },
>>> +    { .compatible = "qcom,sm8450-mdss", .data = &sm8250_data },
>>>       {}
>>>   };
>>>   MODULE_DEVICE_TABLE(of, mdss_dt_match);
> 

Re: [RFC PATCH 1/4] drm/msm/mdss: convert UBWC setup to use match data

[Dmitry Baryshkov]

On 09/01/2023 22:32, Abhinav Kumar wrote:
> 
> 
> On 1/9/2023 12:17 PM, Dmitry Baryshkov wrote:
>> On 09/01/2023 21:53, Abhinav Kumar wrote:
>>>
>>>
>>> On 12/7/2022 4:08 PM, Dmitry Baryshkov wrote:
>>>> To simplify adding new platforms and to make settings more obvious,
>>>> rewrite the UBWC setup to use the data structure to pass platform 
>>>> config
>>>> rather than just calling the functions direcly.
>>>
>>> Why not use the catalog to store this information rather than using 
>>> the platform device match data?
>>>
>>> This seems more appropriate for the catalog.
>>
>> Which catalog?
>>
>> If you are talking about the DPU hw catalog, it's not possible. DPU 
>> and MDSS are two distinct drivers even if they are built into the same 
>> module.
>>
>> And if you are talking about adding mdss_catalog, I'd abstain from 
>> that idea. It is too easy to update one piece and forget the other 
>> one. Using match data is what other drivers are using (and it ensures 
>> that each new supported device gets its correct match data).
>>
> 
> Yes, I was referring to the DPU catalog.
> 
> But now I recall the mess because of the UBWC register being part of 
> mmio base which the DPU doesnt map.
> 
> I do think that the platform match data is a bit of an overkill just to 
> store the UBWC values but the msm_mdss driver today doesnt program 
> anything else today so lets go with this.
> 
> But ... some comments below.
> 
>>>
>>>>
>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>> ---
>>>>   drivers/gpu/drm/msm/msm_mdss.c | 158 
>>>> ++++++++++++++++++++-------------
>>>>   1 file changed, 94 insertions(+), 64 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/msm_mdss.c 
>>>> b/drivers/gpu/drm/msm/msm_mdss.c
>>>> index 92773e0a8fda..2219c1bd59a9 100644
>>>> --- a/drivers/gpu/drm/msm/msm_mdss.c
>>>> +++ b/drivers/gpu/drm/msm/msm_mdss.c
>>>> @@ -29,6 +29,14 @@
>>>>   #define MIN_IB_BW    400000000UL /* Min ib vote 400MB */
>>>> +struct msm_mdss_data {
>>>> +    u32 ubwc_version;
>>>> +    u32 ubwc_swizzle;
>>>> +    u32 ubwc_static;
>>>> +    u32 highest_bank_bit;
>>>> +    u32 macrotile_mode;
>>>> +};
>>>> +
>>>>   struct msm_mdss {
>>>>       struct device *dev;
>>>> @@ -40,6 +48,7 @@ struct msm_mdss {
>>>>           unsigned long enabled_mask;
>>>>           struct irq_domain *domain;
>>>>       } irq_controller;
>>>> +    const struct msm_mdss_data *mdss_data;
>>>>       struct icc_path *path[2];
>>>>       u32 num_paths;
>>>>   };
>>>> @@ -180,46 +189,40 @@ static int _msm_mdss_irq_domain_add(struct 
>>>> msm_mdss *msm_mdss)
>>>>   #define UBWC_3_0 0x30000000
>>>>   #define UBWC_4_0 0x40000000
>>>> -static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss,
>>>> -                       u32 ubwc_static)
>>>> +static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss)
>>>>   {
>>>> -    writel_relaxed(ubwc_static, msm_mdss->mmio + UBWC_STATIC);
>>>> +    const struct msm_mdss_data *data = msm_mdss->mdss_data;
>>>> +
>>>> +    writel_relaxed(data->ubwc_static, msm_mdss->mmio + UBWC_STATIC);
>>>>   }
>>>> -static void msm_mdss_setup_ubwc_dec_30(struct msm_mdss *msm_mdss,
>>>> -                       unsigned int ubwc_version,
>>>> -                       u32 ubwc_swizzle,
>>>> -                       u32 highest_bank_bit,
>>>> -                       u32 macrotile_mode)
>>>> +static void msm_mdss_setup_ubwc_dec_30(struct msm_mdss *msm_mdss)
>>>>   {
>>>> -    u32 value = (ubwc_swizzle & 0x1) |
>>>> -            (highest_bank_bit & 0x3) << 4 |
>>>> -            (macrotile_mode & 0x1) << 12;
>>>> +    const struct msm_mdss_data *data = msm_mdss->mdss_data;
>>>> +    u32 value = (data->ubwc_swizzle & 0x1) |
>>>> +            (data->highest_bank_bit & 0x3) << 4 |
>>>> +            (data->macrotile_mode & 0x1) << 12;
>>>> -    if (ubwc_version == UBWC_3_0)
>>>> +    if (data->ubwc_version == UBWC_3_0)
>>>>           value |= BIT(10);
>>>> -    if (ubwc_version == UBWC_1_0)
>>>> +    if (data->ubwc_version == UBWC_1_0)
>>>>           value |= BIT(8);
>>>>       writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
>>>>   }
>>>> -static void msm_mdss_setup_ubwc_dec_40(struct msm_mdss *msm_mdss,
>>>> -                       unsigned int ubwc_version,
>>>> -                       u32 ubwc_swizzle,
>>>> -                       u32 ubwc_static,
>>>> -                       u32 highest_bank_bit,
>>>> -                       u32 macrotile_mode)
>>>> +static void msm_mdss_setup_ubwc_dec_40(struct msm_mdss *msm_mdss)
>>>>   {
>>>> -    u32 value = (ubwc_swizzle & 0x7) |
>>>> -            (ubwc_static & 0x1) << 3 |
>>>> -            (highest_bank_bit & 0x7) << 4 |
>>>> -            (macrotile_mode & 0x1) << 12;
>>>> +    const struct msm_mdss_data *data = msm_mdss->mdss_data;
>>>> +    u32 value = (data->ubwc_swizzle & 0x7) |
>>>> +            (data->ubwc_static & 0x1) << 3 |
>>>> +            (data->highest_bank_bit & 0x7) << 4 |
>>>> +            (data->macrotile_mode & 0x1) << 12;
>>>>       writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
>>>> -    if (ubwc_version == UBWC_3_0) {
>>>> +    if (data->ubwc_version == UBWC_3_0) {
>>>>           writel_relaxed(1, msm_mdss->mmio + UBWC_CTRL_2);
>>>>           writel_relaxed(0, msm_mdss->mmio + UBWC_PREDICTION_MODE);
>>>>       } else {
>>>> @@ -232,6 +235,7 @@ static int msm_mdss_enable(struct msm_mdss 
>>>> *msm_mdss)
>>>>   {
>>>>       int ret;
>>>>       u32 hw_rev;
>>>> +    u32 ubwc_dec_hw_version;
>>>>       /*
>>>>        * Several components have AXI clocks that can only be turned 
>>>> on if
>>>> @@ -250,53 +254,36 @@ static int msm_mdss_enable(struct msm_mdss 
>>>> *msm_mdss)
>>>>        * HW_REV requires MDSS_MDP_CLK, which is not enabled by the 
>>>> mdss on
>>>>        * mdp5 hardware. Skip reading it for now.
>>>>        */
>>>> -    if (msm_mdss->is_mdp5)
>>>> +    if (msm_mdss->is_mdp5 || !msm_mdss->mdss_data)
>>>>           return 0;
>>>>       hw_rev = readl_relaxed(msm_mdss->mmio + HW_REV);
> 
> hw_rev is not used anymore now so why not just drop that reg read 
> altogether.
> 
>>>>       dev_dbg(msm_mdss->dev, "HW_REV: 0x%x\n", hw_rev);
>>>> +
>>>> +    ubwc_dec_hw_version = readl_relaxed(msm_mdss->mmio + 
>>>> UBWC_DEC_HW_VERSION);
> 
> If we are going to tie UBWC version to the HW compatible match, then 
> even this register read can be skipped and instead you can add 
> ubwc_dec_hw_version to your match data struct and skip this read as well.
> 
> That way we get rid of all register reads in this path which have 
> continuously bugged us with crashes.

But then register writes would bug you with crashes, won't they? I think 
that crashes happen because of missing MDSS_MDP_CLK clock, don't they?

Anyway, this sounds like a good idea, so, let's do it.

> 
>>>>       dev_dbg(msm_mdss->dev, "UBWC_DEC_HW_VERSION: 0x%x\n",
>>>> -        readl_relaxed(msm_mdss->mmio + UBWC_DEC_HW_VERSION));
>>>> +        ubwc_dec_hw_version);
>>>>       /*
>>>>        * ubwc config is part of the "mdss" region which is not 
>>>> accessible
>>>>        * from the rest of the driver. hardcode known configurations 
>>>> here
>>>>        *
>>>>        * Decoder version can be read from the UBWC_DEC_HW_VERSION reg,
>>>> -     * UBWC_n and the rest of params comes from hw_catalog.
>>>> -     * Unforunately this driver can not access hw catalog, so we 
>>>> have to
>>>> -     * hardcode them here.
>>>> +     * UBWC_n and the rest of params comes from hw data.
>>>>        */
>>>> -    switch (hw_rev) {
>>>> -    case DPU_HW_VER_500:
>>>> -    case DPU_HW_VER_501:
>>>> -        msm_mdss_setup_ubwc_dec_30(msm_mdss, UBWC_3_0, 0, 2, 0);
>>>> -        break;
>>>> -    case DPU_HW_VER_600:
>>>> -        /* TODO: highest_bank_bit = 2 for LP_DDR4 */
>>>> -        msm_mdss_setup_ubwc_dec_40(msm_mdss, UBWC_4_0, 6, 1, 3, 1);
>>>> -        break;
>>>> -    case DPU_HW_VER_620:
>>>> -        /* UBWC_2_0 */
>>>> -        msm_mdss_setup_ubwc_dec_20(msm_mdss, 0x1e);
>>>> +    switch (ubwc_dec_hw_version) {
>>>> +    case UBWC_2_0:
>>>> +        msm_mdss_setup_ubwc_dec_20(msm_mdss);
>>>>           break;
>>>> -    case DPU_HW_VER_630:
>>>> -        /* UBWC_2_0 */
>>>> -        msm_mdss_setup_ubwc_dec_20(msm_mdss, 0x11f);
>>>> +    case UBWC_3_0:
>>>> +        msm_mdss_setup_ubwc_dec_30(msm_mdss);
>>>>           break;
>>>> -    case DPU_HW_VER_700:
>>>> -        /* TODO: highest_bank_bit = 2 for LP_DDR4 */
>>>> -        msm_mdss_setup_ubwc_dec_40(msm_mdss, UBWC_4_0, 6, 1, 3, 1);
>>>> +    case UBWC_4_0:
>>>> +        msm_mdss_setup_ubwc_dec_40(msm_mdss);
>>>>           break;
>>>> -    case DPU_HW_VER_720:
>>>> -        msm_mdss_setup_ubwc_dec_40(msm_mdss, UBWC_3_0, 6, 1, 1, 1);
>>>> -        break;
>>>> -    case DPU_HW_VER_800:
>>>> -        msm_mdss_setup_ubwc_dec_40(msm_mdss, UBWC_4_0, 6, 1, 2, 1);
>>>> -        break;
>>>> -    case DPU_HW_VER_810:
>>>> -        /* TODO: highest_bank_bit = 2 for LP_DDR4 */
>>>> -        msm_mdss_setup_ubwc_dec_40(msm_mdss, UBWC_4_0, 6, 1, 3, 1);
>>>> +    default:
>>>> +        dev_err(msm_mdss->dev, "Unuspported UBWC decoder version 
>>>> %x\n",
>>>> +            ubwc_dec_hw_version);
>>>>           break;
>>>>       }
>>>> @@ -487,6 +474,8 @@ static int mdss_probe(struct platform_device *pdev)
>>>>       if (IS_ERR(mdss))
>>>>           return PTR_ERR(mdss);
>>>> +    mdss->mdss_data = of_device_get_match_data(&pdev->dev);
>>>> +
>>>>       platform_set_drvdata(pdev, mdss);
>>>>       /*
>>>> @@ -516,20 +505,61 @@ static int mdss_remove(struct platform_device 
>>>> *pdev)
>>>>       return 0;
>>>>   }
>>>> +static const struct msm_mdss_data sc7180_data = {
>>>> +    .ubwc_version = UBWC_2_0,
>>>> +    .ubwc_static = 0x1e,
>>>> +};
>>>> +
>>>> +static const struct msm_mdss_data sc7280_data = {
>>>> +    .ubwc_version = UBWC_3_0,
>>>> +    .ubwc_swizzle = 6,
>>>> +    .ubwc_static = 1,
>>>> +    .highest_bank_bit = 1,
>>>> +    .macrotile_mode = 1,
>>>> +};
>>>> +
>>>> +static const struct msm_mdss_data sc8280xp_data = {
>>>> +    .ubwc_version = UBWC_4_0,
>>>> +    .ubwc_swizzle = 6,
>>>> +    .ubwc_static = 1,
>>>> +    .highest_bank_bit = 2,
>>>> +    .macrotile_mode = 1,
>>>> +};
>>>> +
>>>> +static const struct msm_mdss_data sm8150_data = {
>>>> +    .ubwc_version = UBWC_3_0,
>>>> +    .highest_bank_bit = 2,
>>>> +};
>>>> +
>>>> +static const struct msm_mdss_data sm6115_data = {
>>>> +    .ubwc_version = UBWC_2_0,
>>>> +    .ubwc_swizzle = 7,
>>>> +    .ubwc_static = 0x11f,
>>>> +};
>>>> +
>>>> +static const struct msm_mdss_data sm8250_data = {
>>>> +    .ubwc_version = UBWC_4_0,
>>>> +    .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" },
>>>>       { .compatible = "qcom,qcm2290-mdss" },
>>>>       { .compatible = "qcom,sdm845-mdss" },
>>>> -    { .compatible = "qcom,sc7180-mdss" },
>>>> -    { .compatible = "qcom,sc7280-mdss" },
>>>> +    { .compatible = "qcom,sc7180-mdss", .data = &sc7180_data },
>>>> +    { .compatible = "qcom,sc7280-mdss", .data = &sc7280_data },
>>>>       { .compatible = "qcom,sc8180x-mdss" },
>>>> -    { .compatible = "qcom,sc8280xp-mdss" },
>>>> -    { .compatible = "qcom,sm6115-mdss" },
>>>> -    { .compatible = "qcom,sm8150-mdss" },
>>>> -    { .compatible = "qcom,sm8250-mdss" },
>>>> -    { .compatible = "qcom,sm8350-mdss" },
>>>> -    { .compatible = "qcom,sm8450-mdss" },
>>>> +    { .compatible = "qcom,sc8280xp-mdss", .data = &sc8280xp_data },
>>>> +    { .compatible = "qcom,sm6115-mdss", .data = &sm6115_data },
>>>> +    { .compatible = "qcom,sm8150-mdss", .data = &sm8150_data },
>>>> +    { .compatible = "qcom,sm8250-mdss", .data = &sm8250_data },
>>>> +    { .compatible = "qcom,sm8350-mdss", .data = &sm8250_data },
>>>> +    { .compatible = "qcom,sm8450-mdss", .data = &sm8250_data },
>>>>       {}
>>>>   };
>>>>   MODULE_DEVICE_TABLE(of, mdss_dt_match);
>>

-- 
With best wishes
Dmitry

Re: [RFC PATCH 1/4] drm/msm/mdss: convert UBWC setup to use match data

[Abhinav Kumar]

On 1/9/2023 1:11 PM, Dmitry Baryshkov wrote:
> On 09/01/2023 22:32, Abhinav Kumar wrote:
>>
>>
>> On 1/9/2023 12:17 PM, Dmitry Baryshkov wrote:
>>> On 09/01/2023 21:53, Abhinav Kumar wrote:
>>>>
>>>>
>>>> On 12/7/2022 4:08 PM, Dmitry Baryshkov wrote:
>>>>> To simplify adding new platforms and to make settings more obvious,
>>>>> rewrite the UBWC setup to use the data structure to pass platform 
>>>>> config
>>>>> rather than just calling the functions direcly.
>>>>
>>>> Why not use the catalog to store this information rather than using 
>>>> the platform device match data?
>>>>
>>>> This seems more appropriate for the catalog.
>>>
>>> Which catalog?
>>>
>>> If you are talking about the DPU hw catalog, it's not possible. DPU 
>>> and MDSS are two distinct drivers even if they are built into the 
>>> same module.
>>>
>>> And if you are talking about adding mdss_catalog, I'd abstain from 
>>> that idea. It is too easy to update one piece and forget the other 
>>> one. Using match data is what other drivers are using (and it ensures 
>>> that each new supported device gets its correct match data).
>>>
>>
>> Yes, I was referring to the DPU catalog.
>>
>> But now I recall the mess because of the UBWC register being part of 
>> mmio base which the DPU doesnt map.
>>
>> I do think that the platform match data is a bit of an overkill just 
>> to store the UBWC values but the msm_mdss driver today doesnt program 
>> anything else today so lets go with this.
>>
>> But ... some comments below.
>>
>>>>
>>>>>
>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>> ---
>>>>>   drivers/gpu/drm/msm/msm_mdss.c | 158 
>>>>> ++++++++++++++++++++-------------
>>>>>   1 file changed, 94 insertions(+), 64 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/msm/msm_mdss.c 
>>>>> b/drivers/gpu/drm/msm/msm_mdss.c
>>>>> index 92773e0a8fda..2219c1bd59a9 100644
>>>>> --- a/drivers/gpu/drm/msm/msm_mdss.c
>>>>> +++ b/drivers/gpu/drm/msm/msm_mdss.c
>>>>> @@ -29,6 +29,14 @@
>>>>>   #define MIN_IB_BW    400000000UL /* Min ib vote 400MB */
>>>>> +struct msm_mdss_data {
>>>>> +    u32 ubwc_version;
>>>>> +    u32 ubwc_swizzle;
>>>>> +    u32 ubwc_static;
>>>>> +    u32 highest_bank_bit;
>>>>> +    u32 macrotile_mode;
>>>>> +};
>>>>> +
>>>>>   struct msm_mdss {
>>>>>       struct device *dev;
>>>>> @@ -40,6 +48,7 @@ struct msm_mdss {
>>>>>           unsigned long enabled_mask;
>>>>>           struct irq_domain *domain;
>>>>>       } irq_controller;
>>>>> +    const struct msm_mdss_data *mdss_data;
>>>>>       struct icc_path *path[2];
>>>>>       u32 num_paths;
>>>>>   };
>>>>> @@ -180,46 +189,40 @@ static int _msm_mdss_irq_domain_add(struct 
>>>>> msm_mdss *msm_mdss)
>>>>>   #define UBWC_3_0 0x30000000
>>>>>   #define UBWC_4_0 0x40000000
>>>>> -static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss,
>>>>> -                       u32 ubwc_static)
>>>>> +static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss)
>>>>>   {
>>>>> -    writel_relaxed(ubwc_static, msm_mdss->mmio + UBWC_STATIC);
>>>>> +    const struct msm_mdss_data *data = msm_mdss->mdss_data;
>>>>> +
>>>>> +    writel_relaxed(data->ubwc_static, msm_mdss->mmio + UBWC_STATIC);
>>>>>   }
>>>>> -static void msm_mdss_setup_ubwc_dec_30(struct msm_mdss *msm_mdss,
>>>>> -                       unsigned int ubwc_version,
>>>>> -                       u32 ubwc_swizzle,
>>>>> -                       u32 highest_bank_bit,
>>>>> -                       u32 macrotile_mode)
>>>>> +static void msm_mdss_setup_ubwc_dec_30(struct msm_mdss *msm_mdss)
>>>>>   {
>>>>> -    u32 value = (ubwc_swizzle & 0x1) |
>>>>> -            (highest_bank_bit & 0x3) << 4 |
>>>>> -            (macrotile_mode & 0x1) << 12;
>>>>> +    const struct msm_mdss_data *data = msm_mdss->mdss_data;
>>>>> +    u32 value = (data->ubwc_swizzle & 0x1) |
>>>>> +            (data->highest_bank_bit & 0x3) << 4 |
>>>>> +            (data->macrotile_mode & 0x1) << 12;
>>>>> -    if (ubwc_version == UBWC_3_0)
>>>>> +    if (data->ubwc_version == UBWC_3_0)
>>>>>           value |= BIT(10);
>>>>> -    if (ubwc_version == UBWC_1_0)
>>>>> +    if (data->ubwc_version == UBWC_1_0)
>>>>>           value |= BIT(8);
>>>>>       writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
>>>>>   }
>>>>> -static void msm_mdss_setup_ubwc_dec_40(struct msm_mdss *msm_mdss,
>>>>> -                       unsigned int ubwc_version,
>>>>> -                       u32 ubwc_swizzle,
>>>>> -                       u32 ubwc_static,
>>>>> -                       u32 highest_bank_bit,
>>>>> -                       u32 macrotile_mode)
>>>>> +static void msm_mdss_setup_ubwc_dec_40(struct msm_mdss *msm_mdss)
>>>>>   {
>>>>> -    u32 value = (ubwc_swizzle & 0x7) |
>>>>> -            (ubwc_static & 0x1) << 3 |
>>>>> -            (highest_bank_bit & 0x7) << 4 |
>>>>> -            (macrotile_mode & 0x1) << 12;
>>>>> +    const struct msm_mdss_data *data = msm_mdss->mdss_data;
>>>>> +    u32 value = (data->ubwc_swizzle & 0x7) |
>>>>> +            (data->ubwc_static & 0x1) << 3 |
>>>>> +            (data->highest_bank_bit & 0x7) << 4 |
>>>>> +            (data->macrotile_mode & 0x1) << 12;
>>>>>       writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
>>>>> -    if (ubwc_version == UBWC_3_0) {
>>>>> +    if (data->ubwc_version == UBWC_3_0) {
>>>>>           writel_relaxed(1, msm_mdss->mmio + UBWC_CTRL_2);
>>>>>           writel_relaxed(0, msm_mdss->mmio + UBWC_PREDICTION_MODE);
>>>>>       } else {
>>>>> @@ -232,6 +235,7 @@ static int msm_mdss_enable(struct msm_mdss 
>>>>> *msm_mdss)
>>>>>   {
>>>>>       int ret;
>>>>>       u32 hw_rev;
>>>>> +    u32 ubwc_dec_hw_version;
>>>>>       /*
>>>>>        * Several components have AXI clocks that can only be turned 
>>>>> on if
>>>>> @@ -250,53 +254,36 @@ static int msm_mdss_enable(struct msm_mdss 
>>>>> *msm_mdss)
>>>>>        * HW_REV requires MDSS_MDP_CLK, which is not enabled by the 
>>>>> mdss on
>>>>>        * mdp5 hardware. Skip reading it for now.
>>>>>        */
>>>>> -    if (msm_mdss->is_mdp5)
>>>>> +    if (msm_mdss->is_mdp5 || !msm_mdss->mdss_data)
>>>>>           return 0;
>>>>>       hw_rev = readl_relaxed(msm_mdss->mmio + HW_REV);
>>
>> hw_rev is not used anymore now so why not just drop that reg read 
>> altogether.
>>
>>>>>       dev_dbg(msm_mdss->dev, "HW_REV: 0x%x\n", hw_rev);
>>>>> +
>>>>> +    ubwc_dec_hw_version = readl_relaxed(msm_mdss->mmio + 
>>>>> UBWC_DEC_HW_VERSION);
>>
>> If we are going to tie UBWC version to the HW compatible match, then 
>> even this register read can be skipped and instead you can add 
>> ubwc_dec_hw_version to your match data struct and skip this read as well.
>>
>> That way we get rid of all register reads in this path which have 
>> continuously bugged us with crashes.
> 
> But then register writes would bug you with crashes, won't they? I think 
> that crashes happen because of missing MDSS_MDP_CLK clock, don't they?

Yes, that issue is also there today too but if the read crashed we dont 
get that far. Yes they were due to missing mdp clock.

In the longer term, I would like to get rid of these register writes too 
and address this comment

/*
          * ubwc config is part of the "mdss" region which is not accessible
          * from the rest of the driver. hardcode known configurations here
          *
          * Decoder version can be read from the UBWC_DEC_HW_VERSION reg,
          * UBWC_n and the rest of params comes from hw_catalog.
          * Unforunately this driver can not access hw catalog, so we 
have to
          * hardcode them here.
          */



> 
> Anyway, this sounds like a good idea, so, let's do it.
> 

Yup atleast removing the reads is a good start.

What do you think of having a msm_kms func to do this. That way it can 
get delegated to a dpu_kms file which has access to the catalog.

Ofcourse, that will be a different change but just thought would ask here.

>>
>>>>>       dev_dbg(msm_mdss->dev, "UBWC_DEC_HW_VERSION: 0x%x\n",
>>>>> -        readl_relaxed(msm_mdss->mmio + UBWC_DEC_HW_VERSION));
>>>>> +        ubwc_dec_hw_version);
>>>>>       /*
>>>>>        * ubwc config is part of the "mdss" region which is not 
>>>>> accessible
>>>>>        * from the rest of the driver. hardcode known configurations 
>>>>> here
>>>>>        *
>>>>>        * Decoder version can be read from the UBWC_DEC_HW_VERSION reg,
>>>>> -     * UBWC_n and the rest of params comes from hw_catalog.
>>>>> -     * Unforunately this driver can not access hw catalog, so we 
>>>>> have to
>>>>> -     * hardcode them here.
>>>>> +     * UBWC_n and the rest of params comes from hw data.
>>>>>        */
>>>>> -    switch (hw_rev) {
>>>>> -    case DPU_HW_VER_500:
>>>>> -    case DPU_HW_VER_501:
>>>>> -        msm_mdss_setup_ubwc_dec_30(msm_mdss, UBWC_3_0, 0, 2, 0);
>>>>> -        break;
>>>>> -    case DPU_HW_VER_600:
>>>>> -        /* TODO: highest_bank_bit = 2 for LP_DDR4 */
>>>>> -        msm_mdss_setup_ubwc_dec_40(msm_mdss, UBWC_4_0, 6, 1, 3, 1);
>>>>> -        break;
>>>>> -    case DPU_HW_VER_620:
>>>>> -        /* UBWC_2_0 */
>>>>> -        msm_mdss_setup_ubwc_dec_20(msm_mdss, 0x1e);
>>>>> +    switch (ubwc_dec_hw_version) {
>>>>> +    case UBWC_2_0:
>>>>> +        msm_mdss_setup_ubwc_dec_20(msm_mdss);
>>>>>           break;
>>>>> -    case DPU_HW_VER_630:
>>>>> -        /* UBWC_2_0 */
>>>>> -        msm_mdss_setup_ubwc_dec_20(msm_mdss, 0x11f);
>>>>> +    case UBWC_3_0:
>>>>> +        msm_mdss_setup_ubwc_dec_30(msm_mdss);
>>>>>           break;
>>>>> -    case DPU_HW_VER_700:
>>>>> -        /* TODO: highest_bank_bit = 2 for LP_DDR4 */
>>>>> -        msm_mdss_setup_ubwc_dec_40(msm_mdss, UBWC_4_0, 6, 1, 3, 1);
>>>>> +    case UBWC_4_0:
>>>>> +        msm_mdss_setup_ubwc_dec_40(msm_mdss);
>>>>>           break;
>>>>> -    case DPU_HW_VER_720:
>>>>> -        msm_mdss_setup_ubwc_dec_40(msm_mdss, UBWC_3_0, 6, 1, 1, 1);
>>>>> -        break;
>>>>> -    case DPU_HW_VER_800:
>>>>> -        msm_mdss_setup_ubwc_dec_40(msm_mdss, UBWC_4_0, 6, 1, 2, 1);
>>>>> -        break;
>>>>> -    case DPU_HW_VER_810:
>>>>> -        /* TODO: highest_bank_bit = 2 for LP_DDR4 */
>>>>> -        msm_mdss_setup_ubwc_dec_40(msm_mdss, UBWC_4_0, 6, 1, 3, 1);
>>>>> +    default:
>>>>> +        dev_err(msm_mdss->dev, "Unuspported UBWC decoder version 
>>>>> %x\n",
>>>>> +            ubwc_dec_hw_version);
>>>>>           break;
>>>>>       }
>>>>> @@ -487,6 +474,8 @@ static int mdss_probe(struct platform_device 
>>>>> *pdev)
>>>>>       if (IS_ERR(mdss))
>>>>>           return PTR_ERR(mdss);
>>>>> +    mdss->mdss_data = of_device_get_match_data(&pdev->dev);
>>>>> +
>>>>>       platform_set_drvdata(pdev, mdss);
>>>>>       /*
>>>>> @@ -516,20 +505,61 @@ static int mdss_remove(struct platform_device 
>>>>> *pdev)
>>>>>       return 0;
>>>>>   }
>>>>> +static const struct msm_mdss_data sc7180_data = {
>>>>> +    .ubwc_version = UBWC_2_0,
>>>>> +    .ubwc_static = 0x1e,
>>>>> +};
>>>>> +
>>>>> +static const struct msm_mdss_data sc7280_data = {
>>>>> +    .ubwc_version = UBWC_3_0,
>>>>> +    .ubwc_swizzle = 6,
>>>>> +    .ubwc_static = 1,
>>>>> +    .highest_bank_bit = 1,
>>>>> +    .macrotile_mode = 1,
>>>>> +};
>>>>> +
>>>>> +static const struct msm_mdss_data sc8280xp_data = {
>>>>> +    .ubwc_version = UBWC_4_0,
>>>>> +    .ubwc_swizzle = 6,
>>>>> +    .ubwc_static = 1,
>>>>> +    .highest_bank_bit = 2,
>>>>> +    .macrotile_mode = 1,
>>>>> +};
>>>>> +
>>>>> +static const struct msm_mdss_data sm8150_data = {
>>>>> +    .ubwc_version = UBWC_3_0,
>>>>> +    .highest_bank_bit = 2,
>>>>> +};
>>>>> +
>>>>> +static const struct msm_mdss_data sm6115_data = {
>>>>> +    .ubwc_version = UBWC_2_0,
>>>>> +    .ubwc_swizzle = 7,
>>>>> +    .ubwc_static = 0x11f,
>>>>> +};
>>>>> +
>>>>> +static const struct msm_mdss_data sm8250_data = {
>>>>> +    .ubwc_version = UBWC_4_0,
>>>>> +    .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" },
>>>>>       { .compatible = "qcom,qcm2290-mdss" },
>>>>>       { .compatible = "qcom,sdm845-mdss" },
>>>>> -    { .compatible = "qcom,sc7180-mdss" },
>>>>> -    { .compatible = "qcom,sc7280-mdss" },
>>>>> +    { .compatible = "qcom,sc7180-mdss", .data = &sc7180_data },
>>>>> +    { .compatible = "qcom,sc7280-mdss", .data = &sc7280_data },
>>>>>       { .compatible = "qcom,sc8180x-mdss" },
>>>>> -    { .compatible = "qcom,sc8280xp-mdss" },
>>>>> -    { .compatible = "qcom,sm6115-mdss" },
>>>>> -    { .compatible = "qcom,sm8150-mdss" },
>>>>> -    { .compatible = "qcom,sm8250-mdss" },
>>>>> -    { .compatible = "qcom,sm8350-mdss" },
>>>>> -    { .compatible = "qcom,sm8450-mdss" },
>>>>> +    { .compatible = "qcom,sc8280xp-mdss", .data = &sc8280xp_data },
>>>>> +    { .compatible = "qcom,sm6115-mdss", .data = &sm6115_data },
>>>>> +    { .compatible = "qcom,sm8150-mdss", .data = &sm8150_data },
>>>>> +    { .compatible = "qcom,sm8250-mdss", .data = &sm8250_data },
>>>>> +    { .compatible = "qcom,sm8350-mdss", .data = &sm8250_data },
>>>>> +    { .compatible = "qcom,sm8450-mdss", .data = &sm8250_data },
>>>>>       {}
>>>>>   };
>>>>>   MODULE_DEVICE_TABLE(of, mdss_dt_match);
>>>
> 

Re: [RFC PATCH 1/4] drm/msm/mdss: convert UBWC setup to use match data

[Marijn Suijten]

On 2023-01-09 12:32:18, Abhinav Kumar wrote:
<snip>
> >> On 12/7/2022 4:08 PM, Dmitry Baryshkov wrote:
<snip>
> >>> +struct msm_mdss_data {
> >>> +    u32 ubwc_version;
> >>> +    u32 ubwc_swizzle;
> >>> +    u32 ubwc_static;
> >>> +    u32 highest_bank_bit;
> >>> +    u32 macrotile_mode;
> >>> +};

This magic struct could really use some documentation, otherwise users
will have no idea what fields to set (or omit) nor what values to use.
For example decoder 2.0 seems to only use ubwc_static as a sort of magic
"we don't know what the bits in UBWC_STATIC mean", whereas decoder 3.0
reconstructs this field entirely from the other parameters.  Decoder 4.0
however does the same, but _also_ embeds this uwbc_static magic value
back into the register value....?

Also read on below about checking "compatibility" between this struct
and the decoder version, and why I feel this struct (versus mandatory
function arguments) makes this struct less robust.

> >>>   struct msm_mdss {
> >>>       struct device *dev;
> >>> @@ -40,6 +48,7 @@ struct msm_mdss {
> >>>           unsigned long enabled_mask;
> >>>           struct irq_domain *domain;
> >>>       } irq_controller;
> >>> +    const struct msm_mdss_data *mdss_data;
> >>>       struct icc_path *path[2];
> >>>       u32 num_paths;
> >>>   };
> >>> @@ -180,46 +189,40 @@ static int _msm_mdss_irq_domain_add(struct 
> >>> msm_mdss *msm_mdss)
> >>>   #define UBWC_3_0 0x30000000
> >>>   #define UBWC_4_0 0x40000000
> >>> -static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss,
> >>> -                       u32 ubwc_static)
> >>> +static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss)
> >>>   {
> >>> -    writel_relaxed(ubwc_static, msm_mdss->mmio + UBWC_STATIC);
> >>> +    const struct msm_mdss_data *data = msm_mdss->mdss_data;
> >>> +
> >>> +    writel_relaxed(data->ubwc_static, msm_mdss->mmio + UBWC_STATIC);
> >>>   }
> >>> -static void msm_mdss_setup_ubwc_dec_30(struct msm_mdss *msm_mdss,
> >>> -                       unsigned int ubwc_version,
> >>> -                       u32 ubwc_swizzle,
> >>> -                       u32 highest_bank_bit,
> >>> -                       u32 macrotile_mode)
> >>> +static void msm_mdss_setup_ubwc_dec_30(struct msm_mdss *msm_mdss)
> >>>   {
> >>> -    u32 value = (ubwc_swizzle & 0x1) |
> >>> -            (highest_bank_bit & 0x3) << 4 |
> >>> -            (macrotile_mode & 0x1) << 12;
> >>> +    const struct msm_mdss_data *data = msm_mdss->mdss_data;
> >>> +    u32 value = (data->ubwc_swizzle & 0x1) |
> >>> +            (data->highest_bank_bit & 0x3) << 4 |
> >>> +            (data->macrotile_mode & 0x1) << 12;
> >>> -    if (ubwc_version == UBWC_3_0)
> >>> +    if (data->ubwc_version == UBWC_3_0)
> >>>           value |= BIT(10);
> >>> -    if (ubwc_version == UBWC_1_0)
> >>> +    if (data->ubwc_version == UBWC_1_0)
> >>>           value |= BIT(8);
> >>>       writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
> >>>   }
> >>> -static void msm_mdss_setup_ubwc_dec_40(struct msm_mdss *msm_mdss,
> >>> -                       unsigned int ubwc_version,
> >>> -                       u32 ubwc_swizzle,
> >>> -                       u32 ubwc_static,
> >>> -                       u32 highest_bank_bit,
> >>> -                       u32 macrotile_mode)
> >>> +static void msm_mdss_setup_ubwc_dec_40(struct msm_mdss *msm_mdss)
> >>>   {
> >>> -    u32 value = (ubwc_swizzle & 0x7) |
> >>> -            (ubwc_static & 0x1) << 3 |
> >>> -            (highest_bank_bit & 0x7) << 4 |
> >>> -            (macrotile_mode & 0x1) << 12;
> >>> +    const struct msm_mdss_data *data = msm_mdss->mdss_data;
> >>> +    u32 value = (data->ubwc_swizzle & 0x7) |
> >>> +            (data->ubwc_static & 0x1) << 3 |
> >>> +            (data->highest_bank_bit & 0x7) << 4 |
> >>> +            (data->macrotile_mode & 0x1) << 12;
> >>>       writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
> >>> -    if (ubwc_version == UBWC_3_0) {
> >>> +    if (data->ubwc_version == UBWC_3_0) {
> >>>           writel_relaxed(1, msm_mdss->mmio + UBWC_CTRL_2);
> >>>           writel_relaxed(0, msm_mdss->mmio + UBWC_PREDICTION_MODE);
> >>>       } else {
> >>> @@ -232,6 +235,7 @@ static int msm_mdss_enable(struct msm_mdss 
> >>> *msm_mdss)
> >>>   {
> >>>       int ret;
> >>>       u32 hw_rev;
> >>> +    u32 ubwc_dec_hw_version;
> >>>       /*
> >>>        * Several components have AXI clocks that can only be turned 
> >>> on if
> >>> @@ -250,53 +254,36 @@ static int msm_mdss_enable(struct msm_mdss 
> >>> *msm_mdss)
> >>>        * HW_REV requires MDSS_MDP_CLK, which is not enabled by the 
> >>> mdss on
> >>>        * mdp5 hardware. Skip reading it for now.
> >>>        */
> >>> -    if (msm_mdss->is_mdp5)
> >>> +    if (msm_mdss->is_mdp5 || !msm_mdss->mdss_data)
> >>>           return 0;
> >>>       hw_rev = readl_relaxed(msm_mdss->mmio + HW_REV);
> 
> hw_rev is not used anymore now so why not just drop that reg read 
> altogether.
> 
> >>>       dev_dbg(msm_mdss->dev, "HW_REV: 0x%x\n", hw_rev);
> >>> +
> >>> +    ubwc_dec_hw_version = readl_relaxed(msm_mdss->mmio + 
> >>> UBWC_DEC_HW_VERSION);
> 
> If we are going to tie UBWC version to the HW compatible match, then 
> even this register read can be skipped and instead you can add 
> ubwc_dec_hw_version to your match data struct and skip this read as well.

I have suggested in IRC to keep this register read, and utilize it to at
least sanity check the configuration.  You are right that the DPU HW
version already describes what UWBC decoder version is used, but we're
are already questioning whether it was ported correctly for SM6115.  A
WARN() that catches a mismatch between what was written in the "catalog"
(or this match table) versus what the hardware reports would have gone a
long way.

This is especially relevant with the new struct where fields are
(un)used depending on the UBWC HW decoder version, making for an extra
exercise to the developer to double-check whether their struct values
are taken into account or not (or if used ones are accidentally
omitted).

- Marijn

> That way we get rid of all register reads in this path which have 
> continuously bugged us with crashes.

<snip>

Re: [RFC PATCH 1/4] drm/msm/mdss: convert UBWC setup to use match data

[Dmitry Baryshkov]

On 11/01/2023 10:44, Marijn Suijten wrote:
> On 2023-01-09 12:32:18, Abhinav Kumar wrote:
> <snip>
>>>> On 12/7/2022 4:08 PM, Dmitry Baryshkov wrote:
> <snip>
>>>>> +struct msm_mdss_data {
>>>>> +    u32 ubwc_version;
>>>>> +    u32 ubwc_swizzle;
>>>>> +    u32 ubwc_static;
>>>>> +    u32 highest_bank_bit;
>>>>> +    u32 macrotile_mode;
>>>>> +};
> 
> This magic struct could really use some documentation, otherwise users
> will have no idea what fields to set (or omit) nor what values to use.
> For example decoder 2.0 seems to only use ubwc_static as a sort of magic
> "we don't know what the bits in UBWC_STATIC mean", whereas decoder 3.0
> reconstructs this field entirely from the other parameters.  Decoder 4.0
> however does the same, but _also_ embeds this uwbc_static magic value
> back into the register value....?

On the bright side these magic values correspond 1:1 to the vendor dtsi 
and to the part of DPU hw catalog. It would be nice to know the bit used 
by decoder 2.0, but I fear that we'd have to resort to wild guesses 
unless Qualcomm decides to disclose that information.

As for dec 4.0 and ubwc_static. I fear that it's just somebody (writing 
downstream DT parsing) reused the ubwc-static name for the bitfield 
which in reality has some sensible name.

> 
> Also read on below about checking "compatibility" between this struct
> and the decoder version, and why I feel this struct (versus mandatory
> function arguments) makes this struct less robust.
> 
>>>>>    struct msm_mdss {
>>>>>        struct device *dev;
>>>>> @@ -40,6 +48,7 @@ struct msm_mdss {
>>>>>            unsigned long enabled_mask;
>>>>>            struct irq_domain *domain;
>>>>>        } irq_controller;
>>>>> +    const struct msm_mdss_data *mdss_data;
>>>>>        struct icc_path *path[2];
>>>>>        u32 num_paths;
>>>>>    };
>>>>> @@ -180,46 +189,40 @@ static int _msm_mdss_irq_domain_add(struct
>>>>> msm_mdss *msm_mdss)
>>>>>    #define UBWC_3_0 0x30000000
>>>>>    #define UBWC_4_0 0x40000000
>>>>> -static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss,
>>>>> -                       u32 ubwc_static)
>>>>> +static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss)
>>>>>    {
>>>>> -    writel_relaxed(ubwc_static, msm_mdss->mmio + UBWC_STATIC);
>>>>> +    const struct msm_mdss_data *data = msm_mdss->mdss_data;
>>>>> +
>>>>> +    writel_relaxed(data->ubwc_static, msm_mdss->mmio + UBWC_STATIC);
>>>>>    }
>>>>> -static void msm_mdss_setup_ubwc_dec_30(struct msm_mdss *msm_mdss,
>>>>> -                       unsigned int ubwc_version,
>>>>> -                       u32 ubwc_swizzle,
>>>>> -                       u32 highest_bank_bit,
>>>>> -                       u32 macrotile_mode)
>>>>> +static void msm_mdss_setup_ubwc_dec_30(struct msm_mdss *msm_mdss)
>>>>>    {
>>>>> -    u32 value = (ubwc_swizzle & 0x1) |
>>>>> -            (highest_bank_bit & 0x3) << 4 |
>>>>> -            (macrotile_mode & 0x1) << 12;
>>>>> +    const struct msm_mdss_data *data = msm_mdss->mdss_data;
>>>>> +    u32 value = (data->ubwc_swizzle & 0x1) |
>>>>> +            (data->highest_bank_bit & 0x3) << 4 |
>>>>> +            (data->macrotile_mode & 0x1) << 12;
>>>>> -    if (ubwc_version == UBWC_3_0)
>>>>> +    if (data->ubwc_version == UBWC_3_0)
>>>>>            value |= BIT(10);
>>>>> -    if (ubwc_version == UBWC_1_0)
>>>>> +    if (data->ubwc_version == UBWC_1_0)
>>>>>            value |= BIT(8);
>>>>>        writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
>>>>>    }
>>>>> -static void msm_mdss_setup_ubwc_dec_40(struct msm_mdss *msm_mdss,
>>>>> -                       unsigned int ubwc_version,
>>>>> -                       u32 ubwc_swizzle,
>>>>> -                       u32 ubwc_static,
>>>>> -                       u32 highest_bank_bit,
>>>>> -                       u32 macrotile_mode)
>>>>> +static void msm_mdss_setup_ubwc_dec_40(struct msm_mdss *msm_mdss)
>>>>>    {
>>>>> -    u32 value = (ubwc_swizzle & 0x7) |
>>>>> -            (ubwc_static & 0x1) << 3 |
>>>>> -            (highest_bank_bit & 0x7) << 4 |
>>>>> -            (macrotile_mode & 0x1) << 12;
>>>>> +    const struct msm_mdss_data *data = msm_mdss->mdss_data;
>>>>> +    u32 value = (data->ubwc_swizzle & 0x7) |
>>>>> +            (data->ubwc_static & 0x1) << 3 |
>>>>> +            (data->highest_bank_bit & 0x7) << 4 |
>>>>> +            (data->macrotile_mode & 0x1) << 12;
>>>>>        writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
>>>>> -    if (ubwc_version == UBWC_3_0) {
>>>>> +    if (data->ubwc_version == UBWC_3_0) {
>>>>>            writel_relaxed(1, msm_mdss->mmio + UBWC_CTRL_2);
>>>>>            writel_relaxed(0, msm_mdss->mmio + UBWC_PREDICTION_MODE);
>>>>>        } else {
>>>>> @@ -232,6 +235,7 @@ static int msm_mdss_enable(struct msm_mdss
>>>>> *msm_mdss)
>>>>>    {
>>>>>        int ret;
>>>>>        u32 hw_rev;
>>>>> +    u32 ubwc_dec_hw_version;
>>>>>        /*
>>>>>         * Several components have AXI clocks that can only be turned
>>>>> on if
>>>>> @@ -250,53 +254,36 @@ static int msm_mdss_enable(struct msm_mdss
>>>>> *msm_mdss)
>>>>>         * HW_REV requires MDSS_MDP_CLK, which is not enabled by the
>>>>> mdss on
>>>>>         * mdp5 hardware. Skip reading it for now.
>>>>>         */
>>>>> -    if (msm_mdss->is_mdp5)
>>>>> +    if (msm_mdss->is_mdp5 || !msm_mdss->mdss_data)
>>>>>            return 0;
>>>>>        hw_rev = readl_relaxed(msm_mdss->mmio + HW_REV);
>>
>> hw_rev is not used anymore now so why not just drop that reg read
>> altogether.
>>
>>>>>        dev_dbg(msm_mdss->dev, "HW_REV: 0x%x\n", hw_rev);
>>>>> +
>>>>> +    ubwc_dec_hw_version = readl_relaxed(msm_mdss->mmio +
>>>>> UBWC_DEC_HW_VERSION);
>>
>> If we are going to tie UBWC version to the HW compatible match, then
>> even this register read can be skipped and instead you can add
>> ubwc_dec_hw_version to your match data struct and skip this read as well.
> 
> I have suggested in IRC to keep this register read, and utilize it to at
> least sanity check the configuration.  You are right that the DPU HW
> version already describes what UWBC decoder version is used, but we're
> are already questioning whether it was ported correctly for SM6115.  A
> WARN() that catches a mismatch between what was written in the "catalog"
> (or this match table) versus what the hardware reports would have gone a
> long way.

Well... Sanity checking here means we do not trust the kernel. And whom 
we can trust then? Note, that for 6115 I had a question regarding the 
ubwc_version stated in the comment, not in the code. I asked for 
UBWC_DEC_HW_VERSION value just to be sure.

> 
> This is especially relevant with the new struct where fields are
> (un)used depending on the UBWC HW decoder version, making for an extra
> exercise to the developer to double-check whether their struct values
> are taken into account or not (or if used ones are accidentally
> omitted).

Granted the overlay between DPU catalog and MDSS device data maybe we 
should make DPU ask MDSS for the ubwc settings.

> 
> - Marijn
> 
>> That way we get rid of all register reads in this path which have
>> continuously bugged us with crashes.
> 
> <snip>

-- 
With best wishes
Dmitry

Re: [RFC PATCH 1/4] drm/msm/mdss: convert UBWC setup to use match data

[Marijn Suijten]

On 2023-01-11 17:11:03, Dmitry Baryshkov wrote:
> On 11/01/2023 10:44, Marijn Suijten wrote:
> > On 2023-01-09 12:32:18, Abhinav Kumar wrote:
> > <snip>
> >>>> On 12/7/2022 4:08 PM, Dmitry Baryshkov wrote:
> > <snip>
> >>>>> +struct msm_mdss_data {
> >>>>> +    u32 ubwc_version;
> >>>>> +    u32 ubwc_swizzle;
> >>>>> +    u32 ubwc_static;
> >>>>> +    u32 highest_bank_bit;
> >>>>> +    u32 macrotile_mode;
> >>>>> +};
> > 
> > This magic struct could really use some documentation, otherwise users
> > will have no idea what fields to set (or omit) nor what values to use.
> > For example decoder 2.0 seems to only use ubwc_static as a sort of magic
> > "we don't know what the bits in UBWC_STATIC mean", whereas decoder 3.0
> > reconstructs this field entirely from the other parameters.  Decoder 4.0
> > however does the same, but _also_ embeds this uwbc_static magic value
> > back into the register value....?
> 
> On the bright side these magic values correspond 1:1 to the vendor dtsi 
> and to the part of DPU hw catalog. It would be nice to know the bit used 
> by decoder 2.0, but I fear that we'd have to resort to wild guesses 
> unless Qualcomm decides to disclose that information.

If downstream has these fields verbatim that makes it easier to
copy-paste, agreed.

> As for dec 4.0 and ubwc_static. I fear that it's just somebody (writing 
> downstream DT parsing) reused the ubwc-static name for the bitfield 
> which in reality has some sensible name.

Yes, ugh.

> > Also read on below about checking "compatibility" between this struct
> > and the decoder version, and why I feel this struct (versus mandatory
> > function arguments) makes this struct less robust.
> > 
> >>>>>    struct msm_mdss {
> >>>>>        struct device *dev;
> >>>>> @@ -40,6 +48,7 @@ struct msm_mdss {
> >>>>>            unsigned long enabled_mask;
> >>>>>            struct irq_domain *domain;
> >>>>>        } irq_controller;
> >>>>> +    const struct msm_mdss_data *mdss_data;
> >>>>>        struct icc_path *path[2];
> >>>>>        u32 num_paths;
> >>>>>    };
> >>>>> @@ -180,46 +189,40 @@ static int _msm_mdss_irq_domain_add(struct
> >>>>> msm_mdss *msm_mdss)
> >>>>>    #define UBWC_3_0 0x30000000
> >>>>>    #define UBWC_4_0 0x40000000
> >>>>> -static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss,
> >>>>> -                       u32 ubwc_static)
> >>>>> +static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss)
> >>>>>    {
> >>>>> -    writel_relaxed(ubwc_static, msm_mdss->mmio + UBWC_STATIC);
> >>>>> +    const struct msm_mdss_data *data = msm_mdss->mdss_data;
> >>>>> +
> >>>>> +    writel_relaxed(data->ubwc_static, msm_mdss->mmio + UBWC_STATIC);
> >>>>>    }
> >>>>> -static void msm_mdss_setup_ubwc_dec_30(struct msm_mdss *msm_mdss,
> >>>>> -                       unsigned int ubwc_version,
> >>>>> -                       u32 ubwc_swizzle,
> >>>>> -                       u32 highest_bank_bit,
> >>>>> -                       u32 macrotile_mode)
> >>>>> +static void msm_mdss_setup_ubwc_dec_30(struct msm_mdss *msm_mdss)
> >>>>>    {
> >>>>> -    u32 value = (ubwc_swizzle & 0x1) |
> >>>>> -            (highest_bank_bit & 0x3) << 4 |
> >>>>> -            (macrotile_mode & 0x1) << 12;
> >>>>> +    const struct msm_mdss_data *data = msm_mdss->mdss_data;
> >>>>> +    u32 value = (data->ubwc_swizzle & 0x1) |
> >>>>> +            (data->highest_bank_bit & 0x3) << 4 |
> >>>>> +            (data->macrotile_mode & 0x1) << 12;
> >>>>> -    if (ubwc_version == UBWC_3_0)
> >>>>> +    if (data->ubwc_version == UBWC_3_0)
> >>>>>            value |= BIT(10);
> >>>>> -    if (ubwc_version == UBWC_1_0)
> >>>>> +    if (data->ubwc_version == UBWC_1_0)
> >>>>>            value |= BIT(8);
> >>>>>        writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
> >>>>>    }
> >>>>> -static void msm_mdss_setup_ubwc_dec_40(struct msm_mdss *msm_mdss,
> >>>>> -                       unsigned int ubwc_version,
> >>>>> -                       u32 ubwc_swizzle,
> >>>>> -                       u32 ubwc_static,
> >>>>> -                       u32 highest_bank_bit,
> >>>>> -                       u32 macrotile_mode)
> >>>>> +static void msm_mdss_setup_ubwc_dec_40(struct msm_mdss *msm_mdss)
> >>>>>    {
> >>>>> -    u32 value = (ubwc_swizzle & 0x7) |
> >>>>> -            (ubwc_static & 0x1) << 3 |
> >>>>> -            (highest_bank_bit & 0x7) << 4 |
> >>>>> -            (macrotile_mode & 0x1) << 12;
> >>>>> +    const struct msm_mdss_data *data = msm_mdss->mdss_data;
> >>>>> +    u32 value = (data->ubwc_swizzle & 0x7) |
> >>>>> +            (data->ubwc_static & 0x1) << 3 |
> >>>>> +            (data->highest_bank_bit & 0x7) << 4 |
> >>>>> +            (data->macrotile_mode & 0x1) << 12;
> >>>>>        writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
> >>>>> -    if (ubwc_version == UBWC_3_0) {
> >>>>> +    if (data->ubwc_version == UBWC_3_0) {
> >>>>>            writel_relaxed(1, msm_mdss->mmio + UBWC_CTRL_2);
> >>>>>            writel_relaxed(0, msm_mdss->mmio + UBWC_PREDICTION_MODE);
> >>>>>        } else {
> >>>>> @@ -232,6 +235,7 @@ static int msm_mdss_enable(struct msm_mdss
> >>>>> *msm_mdss)
> >>>>>    {
> >>>>>        int ret;
> >>>>>        u32 hw_rev;
> >>>>> +    u32 ubwc_dec_hw_version;
> >>>>>        /*
> >>>>>         * Several components have AXI clocks that can only be turned
> >>>>> on if
> >>>>> @@ -250,53 +254,36 @@ static int msm_mdss_enable(struct msm_mdss
> >>>>> *msm_mdss)
> >>>>>         * HW_REV requires MDSS_MDP_CLK, which is not enabled by the
> >>>>> mdss on
> >>>>>         * mdp5 hardware. Skip reading it for now.
> >>>>>         */
> >>>>> -    if (msm_mdss->is_mdp5)
> >>>>> +    if (msm_mdss->is_mdp5 || !msm_mdss->mdss_data)
> >>>>>            return 0;
> >>>>>        hw_rev = readl_relaxed(msm_mdss->mmio + HW_REV);
> >>
> >> hw_rev is not used anymore now so why not just drop that reg read
> >> altogether.
> >>
> >>>>>        dev_dbg(msm_mdss->dev, "HW_REV: 0x%x\n", hw_rev);
> >>>>> +
> >>>>> +    ubwc_dec_hw_version = readl_relaxed(msm_mdss->mmio +
> >>>>> UBWC_DEC_HW_VERSION);
> >>
> >> If we are going to tie UBWC version to the HW compatible match, then
> >> even this register read can be skipped and instead you can add
> >> ubwc_dec_hw_version to your match data struct and skip this read as well.
> > 
> > I have suggested in IRC to keep this register read, and utilize it to at
> > least sanity check the configuration.  You are right that the DPU HW
> > version already describes what UWBC decoder version is used, but we're
> > are already questioning whether it was ported correctly for SM6115.  A
> > WARN() that catches a mismatch between what was written in the "catalog"
> > (or this match table) versus what the hardware reports would have gone a
> > long way.
> 
> Well... Sanity checking here means we do not trust the kernel. And whom 
> we can trust then?

I have no reason to trust the kernel here.  Knowing the read-back value
might even be necessary to decipher the "downstream" kernel, since it
doesn't specify the decoder /hardware/ version but only the data format?

> Note, that for 6115 I had a question regarding the 
> ubwc_version stated in the comment, not in the code. I asked for 
> UBWC_DEC_HW_VERSION value just to be sure.

Right, that is some weird paraphrasing.  Is it the UBWC_2_0 data format
(because there's nothing in dec_20 performing a conditional based on
this) and only coincidentally being the same as the HW decoder version?

> > This is especially relevant with the new struct where fields are
> > (un)used depending on the UBWC HW decoder version, making for an extra
> > exercise to the developer to double-check whether their struct values
> > are taken into account or not (or if used ones are accidentally
> > omitted).
> 
> Granted the overlay between DPU catalog and MDSS device data maybe we 
> should make DPU ask MDSS for the ubwc settings.

Is DPU also holding on to these values?  I was more so referring to the
fact that the HW_DEC version determines what specific fields are read
and what are not, which may not be immediately obvious when adding a
struct instance unless reading the code.

- Marijn

Re: [RFC PATCH 1/4] drm/msm/mdss: convert UBWC setup to use match data

[Abhinav Kumar]

On 1/11/2023 2:21 PM, Marijn Suijten wrote:
> On 2023-01-11 17:11:03, Dmitry Baryshkov wrote:
>> On 11/01/2023 10:44, Marijn Suijten wrote:
>>> On 2023-01-09 12:32:18, Abhinav Kumar wrote:
>>> <snip>
>>>>>> On 12/7/2022 4:08 PM, Dmitry Baryshkov wrote:
>>> <snip>
>>>>>>> +struct msm_mdss_data {
>>>>>>> +    u32 ubwc_version;
>>>>>>> +    u32 ubwc_swizzle;
>>>>>>> +    u32 ubwc_static;
>>>>>>> +    u32 highest_bank_bit;
>>>>>>> +    u32 macrotile_mode;
>>>>>>> +};
>>>
>>> This magic struct could really use some documentation, otherwise users
>>> will have no idea what fields to set (or omit) nor what values to use.
>>> For example decoder 2.0 seems to only use ubwc_static as a sort of magic
>>> "we don't know what the bits in UBWC_STATIC mean", whereas decoder 3.0
>>> reconstructs this field entirely from the other parameters.  Decoder 4.0
>>> however does the same, but _also_ embeds this uwbc_static magic value
>>> back into the register value....?
>>
>> On the bright side these magic values correspond 1:1 to the vendor dtsi
>> and to the part of DPU hw catalog. It would be nice to know the bit used
>> by decoder 2.0, but I fear that we'd have to resort to wild guesses
>> unless Qualcomm decides to disclose that information.
> 
> If downstream has these fields verbatim that makes it easier to
> copy-paste, agreed.
> 
>> As for dec 4.0 and ubwc_static. I fear that it's just somebody (writing
>> downstream DT parsing) reused the ubwc-static name for the bitfield
>> which in reality has some sensible name.
> 
> Yes, ugh.
> 
>>> Also read on below about checking "compatibility" between this struct
>>> and the decoder version, and why I feel this struct (versus mandatory
>>> function arguments) makes this struct less robust.
>>>
>>>>>>>     struct msm_mdss {
>>>>>>>         struct device *dev;
>>>>>>> @@ -40,6 +48,7 @@ struct msm_mdss {
>>>>>>>             unsigned long enabled_mask;
>>>>>>>             struct irq_domain *domain;
>>>>>>>         } irq_controller;
>>>>>>> +    const struct msm_mdss_data *mdss_data;
>>>>>>>         struct icc_path *path[2];
>>>>>>>         u32 num_paths;
>>>>>>>     };
>>>>>>> @@ -180,46 +189,40 @@ static int _msm_mdss_irq_domain_add(struct
>>>>>>> msm_mdss *msm_mdss)
>>>>>>>     #define UBWC_3_0 0x30000000
>>>>>>>     #define UBWC_4_0 0x40000000
>>>>>>> -static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss,
>>>>>>> -                       u32 ubwc_static)
>>>>>>> +static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss)
>>>>>>>     {
>>>>>>> -    writel_relaxed(ubwc_static, msm_mdss->mmio + UBWC_STATIC);
>>>>>>> +    const struct msm_mdss_data *data = msm_mdss->mdss_data;
>>>>>>> +
>>>>>>> +    writel_relaxed(data->ubwc_static, msm_mdss->mmio + UBWC_STATIC);
>>>>>>>     }
>>>>>>> -static void msm_mdss_setup_ubwc_dec_30(struct msm_mdss *msm_mdss,
>>>>>>> -                       unsigned int ubwc_version,
>>>>>>> -                       u32 ubwc_swizzle,
>>>>>>> -                       u32 highest_bank_bit,
>>>>>>> -                       u32 macrotile_mode)
>>>>>>> +static void msm_mdss_setup_ubwc_dec_30(struct msm_mdss *msm_mdss)
>>>>>>>     {
>>>>>>> -    u32 value = (ubwc_swizzle & 0x1) |
>>>>>>> -            (highest_bank_bit & 0x3) << 4 |
>>>>>>> -            (macrotile_mode & 0x1) << 12;
>>>>>>> +    const struct msm_mdss_data *data = msm_mdss->mdss_data;
>>>>>>> +    u32 value = (data->ubwc_swizzle & 0x1) |
>>>>>>> +            (data->highest_bank_bit & 0x3) << 4 |
>>>>>>> +            (data->macrotile_mode & 0x1) << 12;
>>>>>>> -    if (ubwc_version == UBWC_3_0)
>>>>>>> +    if (data->ubwc_version == UBWC_3_0)
>>>>>>>             value |= BIT(10);
>>>>>>> -    if (ubwc_version == UBWC_1_0)
>>>>>>> +    if (data->ubwc_version == UBWC_1_0)
>>>>>>>             value |= BIT(8);
>>>>>>>         writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
>>>>>>>     }
>>>>>>> -static void msm_mdss_setup_ubwc_dec_40(struct msm_mdss *msm_mdss,
>>>>>>> -                       unsigned int ubwc_version,
>>>>>>> -                       u32 ubwc_swizzle,
>>>>>>> -                       u32 ubwc_static,
>>>>>>> -                       u32 highest_bank_bit,
>>>>>>> -                       u32 macrotile_mode)
>>>>>>> +static void msm_mdss_setup_ubwc_dec_40(struct msm_mdss *msm_mdss)
>>>>>>>     {
>>>>>>> -    u32 value = (ubwc_swizzle & 0x7) |
>>>>>>> -            (ubwc_static & 0x1) << 3 |
>>>>>>> -            (highest_bank_bit & 0x7) << 4 |
>>>>>>> -            (macrotile_mode & 0x1) << 12;
>>>>>>> +    const struct msm_mdss_data *data = msm_mdss->mdss_data;
>>>>>>> +    u32 value = (data->ubwc_swizzle & 0x7) |
>>>>>>> +            (data->ubwc_static & 0x1) << 3 |
>>>>>>> +            (data->highest_bank_bit & 0x7) << 4 |
>>>>>>> +            (data->macrotile_mode & 0x1) << 12;
>>>>>>>         writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
>>>>>>> -    if (ubwc_version == UBWC_3_0) {
>>>>>>> +    if (data->ubwc_version == UBWC_3_0) {
>>>>>>>             writel_relaxed(1, msm_mdss->mmio + UBWC_CTRL_2);
>>>>>>>             writel_relaxed(0, msm_mdss->mmio + UBWC_PREDICTION_MODE);
>>>>>>>         } else {
>>>>>>> @@ -232,6 +235,7 @@ static int msm_mdss_enable(struct msm_mdss
>>>>>>> *msm_mdss)
>>>>>>>     {
>>>>>>>         int ret;
>>>>>>>         u32 hw_rev;
>>>>>>> +    u32 ubwc_dec_hw_version;
>>>>>>>         /*
>>>>>>>          * Several components have AXI clocks that can only be turned
>>>>>>> on if
>>>>>>> @@ -250,53 +254,36 @@ static int msm_mdss_enable(struct msm_mdss
>>>>>>> *msm_mdss)
>>>>>>>          * HW_REV requires MDSS_MDP_CLK, which is not enabled by the
>>>>>>> mdss on
>>>>>>>          * mdp5 hardware. Skip reading it for now.
>>>>>>>          */
>>>>>>> -    if (msm_mdss->is_mdp5)
>>>>>>> +    if (msm_mdss->is_mdp5 || !msm_mdss->mdss_data)
>>>>>>>             return 0;
>>>>>>>         hw_rev = readl_relaxed(msm_mdss->mmio + HW_REV);
>>>>
>>>> hw_rev is not used anymore now so why not just drop that reg read
>>>> altogether.
>>>>
>>>>>>>         dev_dbg(msm_mdss->dev, "HW_REV: 0x%x\n", hw_rev);
>>>>>>> +
>>>>>>> +    ubwc_dec_hw_version = readl_relaxed(msm_mdss->mmio +
>>>>>>> UBWC_DEC_HW_VERSION);
>>>>
>>>> If we are going to tie UBWC version to the HW compatible match, then
>>>> even this register read can be skipped and instead you can add
>>>> ubwc_dec_hw_version to your match data struct and skip this read as well.
>>>
>>> I have suggested in IRC to keep this register read, and utilize it to at
>>> least sanity check the configuration.  You are right that the DPU HW
>>> version already describes what UWBC decoder version is used, but we're
>>> are already questioning whether it was ported correctly for SM6115.  A
>>> WARN() that catches a mismatch between what was written in the "catalog"
>>> (or this match table) versus what the hardware reports would have gone a
>>> long way.
>>
>> Well... Sanity checking here means we do not trust the kernel. And whom
>> we can trust then?
> 
> I have no reason to trust the kernel here.  Knowing the read-back value
> might even be necessary to decipher the "downstream" kernel, since it
> doesn't specify the decoder /hardware/ version but only the data format?
> 

Many values in the catalog are also working on the assumption that they 
have been copied over correctly from downstream.

So I dont think we need to rely on reading the revision from HW.

The reason I would like to skip it is because its directly tied to the 
DPU HW version.

Having an extra register read just to make sure downstream values are 
not incorrectly copied over seems like an overkill.

>> Note, that for 6115 I had a question regarding the
>> ubwc_version stated in the comment, not in the code. I asked for
>> UBWC_DEC_HW_VERSION value just to be sure.
> 
> Right, that is some weird paraphrasing.  Is it the UBWC_2_0 data format
> (because there's nothing in dec_20 performing a conditional based on
> this) and only coincidentally being the same as the HW decoder version?
> 
>>> This is especially relevant with the new struct where fields are
>>> (un)used depending on the UBWC HW decoder version, making for an extra
>>> exercise to the developer to double-check whether their struct values
>>> are taken into account or not (or if used ones are accidentally
>>> omitted).
>>
>> Granted the overlay between DPU catalog and MDSS device data maybe we
>> should make DPU ask MDSS for the ubwc settings.
> 
> Is DPU also holding on to these values?  I was more so referring to the
> fact that the HW_DEC version determines what specific fields are read
> and what are not, which may not be immediately obvious when adding a
> struct instance unless reading the code.
> 
> - Marijn

[RFC PATCH 2/4] drm/msm/mdss: correct the ubwc version for sm6115 platform

[Dmitry Baryshkov]

According to the vendor DT, sm6115 has UBWC 1.0, not 2.0.

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

diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
index 2219c1bd59a9..4401f945b966 100644
--- a/drivers/gpu/drm/msm/msm_mdss.c
+++ b/drivers/gpu/drm/msm/msm_mdss.c
@@ -532,7 +532,7 @@ static const struct msm_mdss_data sm8150_data = {
 };
 
 static const struct msm_mdss_data sm6115_data = {
-	.ubwc_version = UBWC_2_0,
+	.ubwc_version = UBWC_1_0,
 	.ubwc_swizzle = 7,
 	.ubwc_static = 0x11f,
 };
-- 
2.35.1

Re: [RFC PATCH 2/4] drm/msm/mdss: correct the ubwc version for sm6115 platform

[Abhinav Kumar]

On 12/7/2022 4:08 PM, Dmitry Baryshkov wrote:
> According to the vendor DT, sm6115 has UBWC 1.0, not 2.0.
> 

Can you please point me to which file you are referring to?

The internal docs I have are still showing 2.0.

> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/msm_mdss.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
> index 2219c1bd59a9..4401f945b966 100644
> --- a/drivers/gpu/drm/msm/msm_mdss.c
> +++ b/drivers/gpu/drm/msm/msm_mdss.c
> @@ -532,7 +532,7 @@ static const struct msm_mdss_data sm8150_data = {
>   };
>   
>   static const struct msm_mdss_data sm6115_data = {
> -	.ubwc_version = UBWC_2_0,
> +	.ubwc_version = UBWC_1_0,
>   	.ubwc_swizzle = 7,
>   	.ubwc_static = 0x11f,
>   };

Re: [RFC PATCH 2/4] drm/msm/mdss: correct the ubwc version for sm6115 platform

[Dmitry Baryshkov]

On Fri, 13 Jan 2023 at 21:47, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> On 12/7/2022 4:08 PM, Dmitry Baryshkov wrote:
> > According to the vendor DT, sm6115 has UBWC 1.0, not 2.0.
> >
>
> Can you please point me to which file you are referring to?
>
> The internal docs I have are still showing 2.0.

If I understood correctly sm6115 = bengal, thus:

https://android.googlesource.com/kernel/msm-extra/devicetree/+/refs/heads/android-msm-bramble-4.19-android11-qpr1/qcom/bengal-sde.dtsi#103

>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >   drivers/gpu/drm/msm/msm_mdss.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
> > index 2219c1bd59a9..4401f945b966 100644
> > --- a/drivers/gpu/drm/msm/msm_mdss.c
> > +++ b/drivers/gpu/drm/msm/msm_mdss.c
> > @@ -532,7 +532,7 @@ static const struct msm_mdss_data sm8150_data = {
> >   };
> >
> >   static const struct msm_mdss_data sm6115_data = {
> > -     .ubwc_version = UBWC_2_0,
> > +     .ubwc_version = UBWC_1_0,
> >       .ubwc_swizzle = 7,
> >       .ubwc_static = 0x11f,
> >   };



-- 
With best wishes
Dmitry

Re: [RFC PATCH 2/4] drm/msm/mdss: correct the ubwc version for sm6115 platform

[Abhinav Kumar]

On 1/13/2023 1:16 PM, Dmitry Baryshkov wrote:
> On Fri, 13 Jan 2023 at 21:47, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>> On 12/7/2022 4:08 PM, Dmitry Baryshkov wrote:
>>> According to the vendor DT, sm6115 has UBWC 1.0, not 2.0.
>>>
>>
>> Can you please point me to which file you are referring to?
>>
>> The internal docs I have are still showing 2.0.
> 
> If I understood correctly sm6115 = bengal, thus:
> 
> https://android.googlesource.com/kernel/msm-extra/devicetree/+/refs/heads/android-msm-bramble-4.19-android11-qpr1/qcom/bengal-sde.dtsi#103

Ok, so the cause of discrepancy was that the overall recommendation 
across sub-systems was to use UWBC 1.0. Thats why the DTSI has 1.0.

Hence, this change LGTM,

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

> 
>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>    drivers/gpu/drm/msm/msm_mdss.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
>>> index 2219c1bd59a9..4401f945b966 100644
>>> --- a/drivers/gpu/drm/msm/msm_mdss.c
>>> +++ b/drivers/gpu/drm/msm/msm_mdss.c
>>> @@ -532,7 +532,7 @@ static const struct msm_mdss_data sm8150_data = {
>>>    };
>>>
>>>    static const struct msm_mdss_data sm6115_data = {
>>> -     .ubwc_version = UBWC_2_0,
>>> +     .ubwc_version = UBWC_1_0,
>>>        .ubwc_swizzle = 7,
>>>        .ubwc_static = 0x11f,
>>>    };
> 
> 
> 

[RFC PATCH 3/4] drm/msm/mdss: add data for sc8180xp

[Dmitry Baryshkov]

Add platform data for sc8180xp based on sdmshrike-sde.dtsi.

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

diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
index 4401f945b966..5e19ec897670 100644
--- a/drivers/gpu/drm/msm/msm_mdss.c
+++ b/drivers/gpu/drm/msm/msm_mdss.c
@@ -518,6 +518,12 @@ static const struct msm_mdss_data sc7280_data = {
 	.macrotile_mode = 1,
 };
 
+static const struct msm_mdss_data sc8180x_data = {
+	.ubwc_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_swizzle = 6,
@@ -553,7 +559,7 @@ static const struct of_device_id mdss_dt_match[] = {
 	{ .compatible = "qcom,sdm845-mdss" },
 	{ .compatible = "qcom,sc7180-mdss", .data = &sc7180_data },
 	{ .compatible = "qcom,sc7280-mdss", .data = &sc7280_data },
-	{ .compatible = "qcom,sc8180x-mdss" },
+	{ .compatible = "qcom,sc8180x-mdss", .data = &sc8180x_data },
 	{ .compatible = "qcom,sc8280xp-mdss", .data = &sc8280xp_data },
 	{ .compatible = "qcom,sm6115-mdss", .data = &sm6115_data },
 	{ .compatible = "qcom,sm8150-mdss", .data = &sm8150_data },
-- 
2.35.1

Re: [RFC PATCH 3/4] drm/msm/mdss: add data for sc8180xp

[Abhinav Kumar]

On 12/7/2022 4:08 PM, Dmitry Baryshkov wrote:
> Add platform data for sc8180xp based on sdmshrike-sde.dtsi.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

This matches up the docs I have, hence,

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

> ---
>   drivers/gpu/drm/msm/msm_mdss.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
> index 4401f945b966..5e19ec897670 100644
> --- a/drivers/gpu/drm/msm/msm_mdss.c
> +++ b/drivers/gpu/drm/msm/msm_mdss.c
> @@ -518,6 +518,12 @@ static const struct msm_mdss_data sc7280_data = {
>   	.macrotile_mode = 1,
>   };
>   
> +static const struct msm_mdss_data sc8180x_data = {
> +	.ubwc_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_swizzle = 6,
> @@ -553,7 +559,7 @@ static const struct of_device_id mdss_dt_match[] = {
>   	{ .compatible = "qcom,sdm845-mdss" },
>   	{ .compatible = "qcom,sc7180-mdss", .data = &sc7180_data },
>   	{ .compatible = "qcom,sc7280-mdss", .data = &sc7280_data },
> -	{ .compatible = "qcom,sc8180x-mdss" },
> +	{ .compatible = "qcom,sc8180x-mdss", .data = &sc8180x_data },
>   	{ .compatible = "qcom,sc8280xp-mdss", .data = &sc8280xp_data },
>   	{ .compatible = "qcom,sm6115-mdss", .data = &sm6115_data },
>   	{ .compatible = "qcom,sm8150-mdss", .data = &sm8150_data },

[RFC PATCH 4/4] drm/msm/mdss: add the sdm845 data for completeness

[Dmitry Baryshkov]

Add the platform data for sdm845 platform.

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

diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
index 5e19ec897670..b2c6e8b12469 100644
--- a/drivers/gpu/drm/msm/msm_mdss.c
+++ b/drivers/gpu/drm/msm/msm_mdss.c
@@ -532,6 +532,11 @@ static const struct msm_mdss_data sc8280xp_data = {
 	.macrotile_mode = 1,
 };
 
+static const struct msm_mdss_data sdm845_data = {
+	.ubwc_version = UBWC_2_0,
+	.highest_bank_bit = 2,
+};
+
 static const struct msm_mdss_data sm8150_data = {
 	.ubwc_version = UBWC_3_0,
 	.highest_bank_bit = 2,
@@ -556,7 +561,7 @@ static const struct of_device_id mdss_dt_match[] = {
 	{ .compatible = "qcom,mdss" },
 	{ .compatible = "qcom,msm8998-mdss" },
 	{ .compatible = "qcom,qcm2290-mdss" },
-	{ .compatible = "qcom,sdm845-mdss" },
+	{ .compatible = "qcom,sdm845-mdss", .data = &sdm845_data },
 	{ .compatible = "qcom,sc7180-mdss", .data = &sc7180_data },
 	{ .compatible = "qcom,sc7280-mdss", .data = &sc7280_data },
 	{ .compatible = "qcom,sc8180x-mdss", .data = &sc8180x_data },
-- 
2.35.1

Re: [RFC PATCH 4/4] drm/msm/mdss: add the sdm845 data for completeness

[Abhinav Kumar]

On 12/7/2022 4:08 PM, Dmitry Baryshkov wrote:
> Add the platform data for sdm845 platform.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>   drivers/gpu/drm/msm/msm_mdss.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
> index 5e19ec897670..b2c6e8b12469 100644
> --- a/drivers/gpu/drm/msm/msm_mdss.c
> +++ b/drivers/gpu/drm/msm/msm_mdss.c
> @@ -532,6 +532,11 @@ static const struct msm_mdss_data sc8280xp_data = {
>   	.macrotile_mode = 1,
>   };
>   
> +static const struct msm_mdss_data sdm845_data = {
> +	.ubwc_version = UBWC_2_0,
> +	.highest_bank_bit = 2,
> +};
> +
>   static const struct msm_mdss_data sm8150_data = {
>   	.ubwc_version = UBWC_3_0,
>   	.highest_bank_bit = 2,
> @@ -556,7 +561,7 @@ static const struct of_device_id mdss_dt_match[] = {
>   	{ .compatible = "qcom,mdss" },
>   	{ .compatible = "qcom,msm8998-mdss" },
>   	{ .compatible = "qcom,qcm2290-mdss" },
> -	{ .compatible = "qcom,sdm845-mdss" },
> +	{ .compatible = "qcom,sdm845-mdss", .data = &sdm845_data },
>   	{ .compatible = "qcom,sc7180-mdss", .data = &sc7180_data },
>   	{ .compatible = "qcom,sc7280-mdss", .data = &sc7280_data },
>   	{ .compatible = "qcom,sc8180x-mdss", .data = &sc8180x_data },