* [PATCH] drm/msm: less magic numbers in msm_mdss_enable
@ 2022-05-31 12:18 ` Dmitry Baryshkov
0 siblings, 0 replies; 32+ messages in thread
From: Dmitry Baryshkov @ 2022-05-31 12:18 UTC (permalink / raw)
To: Rob Clark, Sean Paul, Abhinav Kumar
Cc: Stephen Boyd, David Airlie, Daniel Vetter, Bjorn Andersson,
linux-arm-msm, dri-devel, freedreno
Replace magic register writes in msm_mdss_enable() with version that
contains less magic and more variable names that can be traced back to
the dpu_hw_catalog or the downstream dtsi files.
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
drivers/gpu/drm/msm/msm_mdss.c | 79 ++++++++++++++++++++++++++++++----
1 file changed, 71 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
index 0454a571adf7..2a48263cd1b5 100644
--- a/drivers/gpu/drm/msm/msm_mdss.c
+++ b/drivers/gpu/drm/msm/msm_mdss.c
@@ -21,6 +21,7 @@
#define HW_REV 0x0
#define HW_INTR_STATUS 0x0010
+#define UBWC_DEC_HW_VERSION 0x58
#define UBWC_STATIC 0x144
#define UBWC_CTRL_2 0x150
#define UBWC_PREDICTION_MODE 0x154
@@ -132,9 +133,63 @@ static int _msm_mdss_irq_domain_add(struct msm_mdss *msm_mdss)
return 0;
}
+#define UBWC_1_0 0x10000000
+#define UBWC_2_0 0x20000000
+#define UBWC_3_0 0x30000000
+#define UBWC_4_0 0x40000000
+
+static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss,
+ u32 ubwc_static)
+{
+ writel_relaxed(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)
+{
+ u32 value = (ubwc_swizzle & 0x1) |
+ (highest_bank_bit & 0x3) << 4 |
+ (macrotile_mode & 0x1) << 12;
+
+ if (ubwc_version == UBWC_3_0)
+ value |= BIT(10);
+
+ if (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)
+{
+ u32 value = (ubwc_swizzle & 0x7) |
+ (ubwc_static & 0x1) << 3 |
+ (highest_bank_bit & 0x7) << 4 |
+ (macrotile_mode & 0x1) << 12;
+
+ writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
+
+ if (ubwc_version == UBWC_3_0) {
+ writel_relaxed(1, msm_mdss->mmio + UBWC_CTRL_2);
+ writel_relaxed(0, msm_mdss->mmio + UBWC_PREDICTION_MODE);
+ } else {
+ writel_relaxed(2, msm_mdss->mmio + UBWC_CTRL_2);
+ writel_relaxed(1, msm_mdss->mmio + UBWC_PREDICTION_MODE);
+ }
+}
+
static int msm_mdss_enable(struct msm_mdss *msm_mdss)
{
int ret;
+ u32 hw_rev;
ret = clk_bulk_prepare_enable(msm_mdss->num_clocks, msm_mdss->clocks);
if (ret) {
@@ -149,26 +204,34 @@ static int msm_mdss_enable(struct msm_mdss *msm_mdss)
if (msm_mdss->is_mdp5)
return 0;
+ hw_rev = readl_relaxed(msm_mdss->mmio + HW_REV);
+ dev_info(msm_mdss->dev, "HW_REV: 0x%x\n", hw_rev);
+ dev_info(msm_mdss->dev, "UBWC_DEC_HW_VERSION: 0x%x\n",
+ readl_relaxed(msm_mdss->mmio + 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 comes from hw_catalog.
+ * Unforunately this driver can not access hw catalog.
*/
- switch (readl_relaxed(msm_mdss->mmio + HW_REV)) {
+ switch (hw_rev) {
case DPU_HW_VER_500:
case DPU_HW_VER_501:
- writel_relaxed(0x420, msm_mdss->mmio + UBWC_STATIC);
+ msm_mdss_setup_ubwc_dec_30(msm_mdss, UBWC_3_0, 0, 2, 0);
break;
case DPU_HW_VER_600:
- /* TODO: 0x102e for LP_DDR4 */
- writel_relaxed(0x103e, msm_mdss->mmio + UBWC_STATIC);
- writel_relaxed(2, msm_mdss->mmio + UBWC_CTRL_2);
- writel_relaxed(1, msm_mdss->mmio + UBWC_PREDICTION_MODE);
+ /* 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:
- writel_relaxed(0x1e, msm_mdss->mmio + UBWC_STATIC);
+ /* UBWC_2_0 */
+ msm_mdss_setup_ubwc_dec_20(msm_mdss, 0x1e);
break;
case DPU_HW_VER_720:
- writel_relaxed(0x101e, msm_mdss->mmio + UBWC_STATIC);
+ msm_mdss_setup_ubwc_dec_40(msm_mdss, UBWC_3_0, 6, 1, 1, 1);
break;
}
--
2.35.1
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH] drm/msm: less magic numbers in msm_mdss_enable
@ 2022-05-31 12:18 ` Dmitry Baryshkov
0 siblings, 0 replies; 32+ messages in thread
From: Dmitry Baryshkov @ 2022-05-31 12:18 UTC (permalink / raw)
To: Rob Clark, Sean Paul, Abhinav Kumar
Cc: David Airlie, linux-arm-msm, dri-devel, Bjorn Andersson,
Stephen Boyd, freedreno
Replace magic register writes in msm_mdss_enable() with version that
contains less magic and more variable names that can be traced back to
the dpu_hw_catalog or the downstream dtsi files.
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
drivers/gpu/drm/msm/msm_mdss.c | 79 ++++++++++++++++++++++++++++++----
1 file changed, 71 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
index 0454a571adf7..2a48263cd1b5 100644
--- a/drivers/gpu/drm/msm/msm_mdss.c
+++ b/drivers/gpu/drm/msm/msm_mdss.c
@@ -21,6 +21,7 @@
#define HW_REV 0x0
#define HW_INTR_STATUS 0x0010
+#define UBWC_DEC_HW_VERSION 0x58
#define UBWC_STATIC 0x144
#define UBWC_CTRL_2 0x150
#define UBWC_PREDICTION_MODE 0x154
@@ -132,9 +133,63 @@ static int _msm_mdss_irq_domain_add(struct msm_mdss *msm_mdss)
return 0;
}
+#define UBWC_1_0 0x10000000
+#define UBWC_2_0 0x20000000
+#define UBWC_3_0 0x30000000
+#define UBWC_4_0 0x40000000
+
+static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss,
+ u32 ubwc_static)
+{
+ writel_relaxed(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)
+{
+ u32 value = (ubwc_swizzle & 0x1) |
+ (highest_bank_bit & 0x3) << 4 |
+ (macrotile_mode & 0x1) << 12;
+
+ if (ubwc_version == UBWC_3_0)
+ value |= BIT(10);
+
+ if (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)
+{
+ u32 value = (ubwc_swizzle & 0x7) |
+ (ubwc_static & 0x1) << 3 |
+ (highest_bank_bit & 0x7) << 4 |
+ (macrotile_mode & 0x1) << 12;
+
+ writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
+
+ if (ubwc_version == UBWC_3_0) {
+ writel_relaxed(1, msm_mdss->mmio + UBWC_CTRL_2);
+ writel_relaxed(0, msm_mdss->mmio + UBWC_PREDICTION_MODE);
+ } else {
+ writel_relaxed(2, msm_mdss->mmio + UBWC_CTRL_2);
+ writel_relaxed(1, msm_mdss->mmio + UBWC_PREDICTION_MODE);
+ }
+}
+
static int msm_mdss_enable(struct msm_mdss *msm_mdss)
{
int ret;
+ u32 hw_rev;
ret = clk_bulk_prepare_enable(msm_mdss->num_clocks, msm_mdss->clocks);
if (ret) {
@@ -149,26 +204,34 @@ static int msm_mdss_enable(struct msm_mdss *msm_mdss)
if (msm_mdss->is_mdp5)
return 0;
+ hw_rev = readl_relaxed(msm_mdss->mmio + HW_REV);
+ dev_info(msm_mdss->dev, "HW_REV: 0x%x\n", hw_rev);
+ dev_info(msm_mdss->dev, "UBWC_DEC_HW_VERSION: 0x%x\n",
+ readl_relaxed(msm_mdss->mmio + 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 comes from hw_catalog.
+ * Unforunately this driver can not access hw catalog.
*/
- switch (readl_relaxed(msm_mdss->mmio + HW_REV)) {
+ switch (hw_rev) {
case DPU_HW_VER_500:
case DPU_HW_VER_501:
- writel_relaxed(0x420, msm_mdss->mmio + UBWC_STATIC);
+ msm_mdss_setup_ubwc_dec_30(msm_mdss, UBWC_3_0, 0, 2, 0);
break;
case DPU_HW_VER_600:
- /* TODO: 0x102e for LP_DDR4 */
- writel_relaxed(0x103e, msm_mdss->mmio + UBWC_STATIC);
- writel_relaxed(2, msm_mdss->mmio + UBWC_CTRL_2);
- writel_relaxed(1, msm_mdss->mmio + UBWC_PREDICTION_MODE);
+ /* 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:
- writel_relaxed(0x1e, msm_mdss->mmio + UBWC_STATIC);
+ /* UBWC_2_0 */
+ msm_mdss_setup_ubwc_dec_20(msm_mdss, 0x1e);
break;
case DPU_HW_VER_720:
- writel_relaxed(0x101e, msm_mdss->mmio + UBWC_STATIC);
+ msm_mdss_setup_ubwc_dec_40(msm_mdss, UBWC_3_0, 6, 1, 1, 1);
break;
}
--
2.35.1
^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH] drm/msm: less magic numbers in msm_mdss_enable
2022-05-31 12:18 ` Dmitry Baryshkov
@ 2022-05-31 22:00 ` Abhinav Kumar
-1 siblings, 0 replies; 32+ messages in thread
From: Abhinav Kumar @ 2022-05-31 22:00 UTC (permalink / raw)
To: Dmitry Baryshkov, Rob Clark, Sean Paul
Cc: Stephen Boyd, David Airlie, Daniel Vetter, Bjorn Andersson,
linux-arm-msm, dri-devel, freedreno
On 5/31/2022 5:18 AM, Dmitry Baryshkov wrote:
> Replace magic register writes in msm_mdss_enable() with version that
> contains less magic and more variable names that can be traced back to
> the dpu_hw_catalog or the downstream dtsi files.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> drivers/gpu/drm/msm/msm_mdss.c | 79 ++++++++++++++++++++++++++++++----
> 1 file changed, 71 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
> index 0454a571adf7..2a48263cd1b5 100644
> --- a/drivers/gpu/drm/msm/msm_mdss.c
> +++ b/drivers/gpu/drm/msm/msm_mdss.c
> @@ -21,6 +21,7 @@
> #define HW_REV 0x0
> #define HW_INTR_STATUS 0x0010
>
> +#define UBWC_DEC_HW_VERSION 0x58
> #define UBWC_STATIC 0x144
> #define UBWC_CTRL_2 0x150
> #define UBWC_PREDICTION_MODE 0x154
> @@ -132,9 +133,63 @@ static int _msm_mdss_irq_domain_add(struct msm_mdss *msm_mdss)
> return 0;
> }
>
> +#define UBWC_1_0 0x10000000
> +#define UBWC_2_0 0x20000000
> +#define UBWC_3_0 0x30000000
> +#define UBWC_4_0 0x40000000
> +
> +static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss,
> + u32 ubwc_static)
> +{
> + writel_relaxed(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)
> +{
> + u32 value = (ubwc_swizzle & 0x1) |
> + (highest_bank_bit & 0x3) << 4 |
> + (macrotile_mode & 0x1) << 12;
> +
> + if (ubwc_version == UBWC_3_0)
> + value |= BIT(10);
> +
> + if (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)
> +{
> + u32 value = (ubwc_swizzle & 0x7) |
> + (ubwc_static & 0x1) << 3 |
> + (highest_bank_bit & 0x7) << 4 |
> + (macrotile_mode & 0x1) << 12;
> +
> + writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
> +
> + if (ubwc_version == UBWC_3_0) {
> + writel_relaxed(1, msm_mdss->mmio + UBWC_CTRL_2);
> + writel_relaxed(0, msm_mdss->mmio + UBWC_PREDICTION_MODE);
> + } else {
> + writel_relaxed(2, msm_mdss->mmio + UBWC_CTRL_2);
> + writel_relaxed(1, msm_mdss->mmio + UBWC_PREDICTION_MODE);
> + }
> +}
> +
Is it possible to unify the above functions by having the internal
ubwc_version checks?
It seems like msm_mdss_setup_ubwc_dec_xxx can keep growing.
I have not looked into each bit programming but from the top level so
feel free to correct if wrong but it seems both do write UBWC_STATIC
(different values based on different UBWC versions) and write some extra
registers based on version
> static int msm_mdss_enable(struct msm_mdss *msm_mdss)
> {
> int ret;
> + u32 hw_rev;
>
> ret = clk_bulk_prepare_enable(msm_mdss->num_clocks, msm_mdss->clocks);
> if (ret) {
> @@ -149,26 +204,34 @@ static int msm_mdss_enable(struct msm_mdss *msm_mdss)
> if (msm_mdss->is_mdp5)
> return 0;
>
> + hw_rev = readl_relaxed(msm_mdss->mmio + HW_REV);
> + dev_info(msm_mdss->dev, "HW_REV: 0x%x\n", hw_rev);
> + dev_info(msm_mdss->dev, "UBWC_DEC_HW_VERSION: 0x%x\n",
> + readl_relaxed(msm_mdss->mmio + UBWC_DEC_HW_VERSION));
we are already printing the HW version here
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c#L1096
Do you want to remove that print then?
> +
> /*
> * 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 comes from hw_catalog.
> + * Unforunately this driver can not access hw catalog.
> */
> - switch (readl_relaxed(msm_mdss->mmio + HW_REV)) {
> + switch (hw_rev) {
> case DPU_HW_VER_500:
> case DPU_HW_VER_501:
> - writel_relaxed(0x420, msm_mdss->mmio + UBWC_STATIC);
> + msm_mdss_setup_ubwc_dec_30(msm_mdss, UBWC_3_0, 0, 2, 0);
> break;
> case DPU_HW_VER_600:
> - /* TODO: 0x102e for LP_DDR4 */
> - writel_relaxed(0x103e, msm_mdss->mmio + UBWC_STATIC);
> - writel_relaxed(2, msm_mdss->mmio + UBWC_CTRL_2);
> - writel_relaxed(1, msm_mdss->mmio + UBWC_PREDICTION_MODE);
> + /* 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:
> - writel_relaxed(0x1e, msm_mdss->mmio + UBWC_STATIC);
> + /* UBWC_2_0 */
> + msm_mdss_setup_ubwc_dec_20(msm_mdss, 0x1e);
> break;
> case DPU_HW_VER_720:
> - writel_relaxed(0x101e, msm_mdss->mmio + UBWC_STATIC);
> + msm_mdss_setup_ubwc_dec_40(msm_mdss, UBWC_3_0, 6, 1, 1, 1);
> break;
> }
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] drm/msm: less magic numbers in msm_mdss_enable
@ 2022-05-31 22:00 ` Abhinav Kumar
0 siblings, 0 replies; 32+ messages in thread
From: Abhinav Kumar @ 2022-05-31 22:00 UTC (permalink / raw)
To: Dmitry Baryshkov, Rob Clark, Sean Paul
Cc: David Airlie, linux-arm-msm, dri-devel, Bjorn Andersson,
Stephen Boyd, freedreno
On 5/31/2022 5:18 AM, Dmitry Baryshkov wrote:
> Replace magic register writes in msm_mdss_enable() with version that
> contains less magic and more variable names that can be traced back to
> the dpu_hw_catalog or the downstream dtsi files.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> drivers/gpu/drm/msm/msm_mdss.c | 79 ++++++++++++++++++++++++++++++----
> 1 file changed, 71 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
> index 0454a571adf7..2a48263cd1b5 100644
> --- a/drivers/gpu/drm/msm/msm_mdss.c
> +++ b/drivers/gpu/drm/msm/msm_mdss.c
> @@ -21,6 +21,7 @@
> #define HW_REV 0x0
> #define HW_INTR_STATUS 0x0010
>
> +#define UBWC_DEC_HW_VERSION 0x58
> #define UBWC_STATIC 0x144
> #define UBWC_CTRL_2 0x150
> #define UBWC_PREDICTION_MODE 0x154
> @@ -132,9 +133,63 @@ static int _msm_mdss_irq_domain_add(struct msm_mdss *msm_mdss)
> return 0;
> }
>
> +#define UBWC_1_0 0x10000000
> +#define UBWC_2_0 0x20000000
> +#define UBWC_3_0 0x30000000
> +#define UBWC_4_0 0x40000000
> +
> +static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss,
> + u32 ubwc_static)
> +{
> + writel_relaxed(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)
> +{
> + u32 value = (ubwc_swizzle & 0x1) |
> + (highest_bank_bit & 0x3) << 4 |
> + (macrotile_mode & 0x1) << 12;
> +
> + if (ubwc_version == UBWC_3_0)
> + value |= BIT(10);
> +
> + if (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)
> +{
> + u32 value = (ubwc_swizzle & 0x7) |
> + (ubwc_static & 0x1) << 3 |
> + (highest_bank_bit & 0x7) << 4 |
> + (macrotile_mode & 0x1) << 12;
> +
> + writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
> +
> + if (ubwc_version == UBWC_3_0) {
> + writel_relaxed(1, msm_mdss->mmio + UBWC_CTRL_2);
> + writel_relaxed(0, msm_mdss->mmio + UBWC_PREDICTION_MODE);
> + } else {
> + writel_relaxed(2, msm_mdss->mmio + UBWC_CTRL_2);
> + writel_relaxed(1, msm_mdss->mmio + UBWC_PREDICTION_MODE);
> + }
> +}
> +
Is it possible to unify the above functions by having the internal
ubwc_version checks?
It seems like msm_mdss_setup_ubwc_dec_xxx can keep growing.
I have not looked into each bit programming but from the top level so
feel free to correct if wrong but it seems both do write UBWC_STATIC
(different values based on different UBWC versions) and write some extra
registers based on version
> static int msm_mdss_enable(struct msm_mdss *msm_mdss)
> {
> int ret;
> + u32 hw_rev;
>
> ret = clk_bulk_prepare_enable(msm_mdss->num_clocks, msm_mdss->clocks);
> if (ret) {
> @@ -149,26 +204,34 @@ static int msm_mdss_enable(struct msm_mdss *msm_mdss)
> if (msm_mdss->is_mdp5)
> return 0;
>
> + hw_rev = readl_relaxed(msm_mdss->mmio + HW_REV);
> + dev_info(msm_mdss->dev, "HW_REV: 0x%x\n", hw_rev);
> + dev_info(msm_mdss->dev, "UBWC_DEC_HW_VERSION: 0x%x\n",
> + readl_relaxed(msm_mdss->mmio + UBWC_DEC_HW_VERSION));
we are already printing the HW version here
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c#L1096
Do you want to remove that print then?
> +
> /*
> * 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 comes from hw_catalog.
> + * Unforunately this driver can not access hw catalog.
> */
> - switch (readl_relaxed(msm_mdss->mmio + HW_REV)) {
> + switch (hw_rev) {
> case DPU_HW_VER_500:
> case DPU_HW_VER_501:
> - writel_relaxed(0x420, msm_mdss->mmio + UBWC_STATIC);
> + msm_mdss_setup_ubwc_dec_30(msm_mdss, UBWC_3_0, 0, 2, 0);
> break;
> case DPU_HW_VER_600:
> - /* TODO: 0x102e for LP_DDR4 */
> - writel_relaxed(0x103e, msm_mdss->mmio + UBWC_STATIC);
> - writel_relaxed(2, msm_mdss->mmio + UBWC_CTRL_2);
> - writel_relaxed(1, msm_mdss->mmio + UBWC_PREDICTION_MODE);
> + /* 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:
> - writel_relaxed(0x1e, msm_mdss->mmio + UBWC_STATIC);
> + /* UBWC_2_0 */
> + msm_mdss_setup_ubwc_dec_20(msm_mdss, 0x1e);
> break;
> case DPU_HW_VER_720:
> - writel_relaxed(0x101e, msm_mdss->mmio + UBWC_STATIC);
> + msm_mdss_setup_ubwc_dec_40(msm_mdss, UBWC_3_0, 6, 1, 1, 1);
> break;
> }
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] drm/msm: less magic numbers in msm_mdss_enable
2022-05-31 22:00 ` Abhinav Kumar
@ 2022-06-01 9:46 ` Dmitry Baryshkov
-1 siblings, 0 replies; 32+ messages in thread
From: Dmitry Baryshkov @ 2022-06-01 9:46 UTC (permalink / raw)
To: Abhinav Kumar
Cc: freedreno, David Airlie, linux-arm-msm, dri-devel,
Bjorn Andersson, Stephen Boyd, Sean Paul
On Wed, 1 Jun 2022 at 01:01, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> On 5/31/2022 5:18 AM, Dmitry Baryshkov wrote:
> > Replace magic register writes in msm_mdss_enable() with version that
> > contains less magic and more variable names that can be traced back to
> > the dpu_hw_catalog or the downstream dtsi files.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> > drivers/gpu/drm/msm/msm_mdss.c | 79 ++++++++++++++++++++++++++++++----
> > 1 file changed, 71 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
> > index 0454a571adf7..2a48263cd1b5 100644
> > --- a/drivers/gpu/drm/msm/msm_mdss.c
> > +++ b/drivers/gpu/drm/msm/msm_mdss.c
> > @@ -21,6 +21,7 @@
> > #define HW_REV 0x0
> > #define HW_INTR_STATUS 0x0010
> >
> > +#define UBWC_DEC_HW_VERSION 0x58
> > #define UBWC_STATIC 0x144
> > #define UBWC_CTRL_2 0x150
> > #define UBWC_PREDICTION_MODE 0x154
> > @@ -132,9 +133,63 @@ static int _msm_mdss_irq_domain_add(struct msm_mdss *msm_mdss)
> > return 0;
> > }
> >
> > +#define UBWC_1_0 0x10000000
> > +#define UBWC_2_0 0x20000000
> > +#define UBWC_3_0 0x30000000
> > +#define UBWC_4_0 0x40000000
> > +
> > +static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss,
> > + u32 ubwc_static)
> > +{
> > + writel_relaxed(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)
> > +{
> > + u32 value = (ubwc_swizzle & 0x1) |
> > + (highest_bank_bit & 0x3) << 4 |
> > + (macrotile_mode & 0x1) << 12;
> > +
> > + if (ubwc_version == UBWC_3_0)
> > + value |= BIT(10);
> > +
> > + if (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)
> > +{
> > + u32 value = (ubwc_swizzle & 0x7) |
> > + (ubwc_static & 0x1) << 3 |
> > + (highest_bank_bit & 0x7) << 4 |
> > + (macrotile_mode & 0x1) << 12;
> > +
> > + writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
> > +
> > + if (ubwc_version == UBWC_3_0) {
> > + writel_relaxed(1, msm_mdss->mmio + UBWC_CTRL_2);
> > + writel_relaxed(0, msm_mdss->mmio + UBWC_PREDICTION_MODE);
> > + } else {
> > + writel_relaxed(2, msm_mdss->mmio + UBWC_CTRL_2);
> > + writel_relaxed(1, msm_mdss->mmio + UBWC_PREDICTION_MODE);
> > + }
> > +}
> > +
>
> Is it possible to unify the above functions by having the internal
> ubwc_version checks?
Note, it's not the ubwc_version, it is the ubwc_dec_hw_version. And
also different functions take different sets of arguments.
> It seems like msm_mdss_setup_ubwc_dec_xxx can keep growing.
>
> I have not looked into each bit programming but from the top level so
> feel free to correct if wrong but it seems both do write UBWC_STATIC
> (different values based on different UBWC versions) and write some extra
> registers based on version
This is what both the current code and the downstream do. See
https://github.com/MiCode/Xiaomi_Kernel_OpenSource/blob/zeus-s-oss/techpack/display-drivers/msm/sde/sde_hw_top.c#L312
>
> > static int msm_mdss_enable(struct msm_mdss *msm_mdss)
> > {
> > int ret;
> > + u32 hw_rev;
> >
> > ret = clk_bulk_prepare_enable(msm_mdss->num_clocks, msm_mdss->clocks);
> > if (ret) {
> > @@ -149,26 +204,34 @@ static int msm_mdss_enable(struct msm_mdss *msm_mdss)
> > if (msm_mdss->is_mdp5)
> > return 0;
> >
> > + hw_rev = readl_relaxed(msm_mdss->mmio + HW_REV);
> > + dev_info(msm_mdss->dev, "HW_REV: 0x%x\n", hw_rev);
> > + dev_info(msm_mdss->dev, "UBWC_DEC_HW_VERSION: 0x%x\n",
> > + readl_relaxed(msm_mdss->mmio + UBWC_DEC_HW_VERSION));
>
> we are already printing the HW version here
>
> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c#L1096
>
> Do you want to remove that print then? May be. Let me take a look.
[skipped]
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] drm/msm: less magic numbers in msm_mdss_enable
@ 2022-06-01 9:46 ` Dmitry Baryshkov
0 siblings, 0 replies; 32+ messages in thread
From: Dmitry Baryshkov @ 2022-06-01 9:46 UTC (permalink / raw)
To: Abhinav Kumar
Cc: Rob Clark, Sean Paul, Stephen Boyd, David Airlie, Daniel Vetter,
Bjorn Andersson, linux-arm-msm, dri-devel, freedreno
On Wed, 1 Jun 2022 at 01:01, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> On 5/31/2022 5:18 AM, Dmitry Baryshkov wrote:
> > Replace magic register writes in msm_mdss_enable() with version that
> > contains less magic and more variable names that can be traced back to
> > the dpu_hw_catalog or the downstream dtsi files.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> > drivers/gpu/drm/msm/msm_mdss.c | 79 ++++++++++++++++++++++++++++++----
> > 1 file changed, 71 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
> > index 0454a571adf7..2a48263cd1b5 100644
> > --- a/drivers/gpu/drm/msm/msm_mdss.c
> > +++ b/drivers/gpu/drm/msm/msm_mdss.c
> > @@ -21,6 +21,7 @@
> > #define HW_REV 0x0
> > #define HW_INTR_STATUS 0x0010
> >
> > +#define UBWC_DEC_HW_VERSION 0x58
> > #define UBWC_STATIC 0x144
> > #define UBWC_CTRL_2 0x150
> > #define UBWC_PREDICTION_MODE 0x154
> > @@ -132,9 +133,63 @@ static int _msm_mdss_irq_domain_add(struct msm_mdss *msm_mdss)
> > return 0;
> > }
> >
> > +#define UBWC_1_0 0x10000000
> > +#define UBWC_2_0 0x20000000
> > +#define UBWC_3_0 0x30000000
> > +#define UBWC_4_0 0x40000000
> > +
> > +static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss,
> > + u32 ubwc_static)
> > +{
> > + writel_relaxed(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)
> > +{
> > + u32 value = (ubwc_swizzle & 0x1) |
> > + (highest_bank_bit & 0x3) << 4 |
> > + (macrotile_mode & 0x1) << 12;
> > +
> > + if (ubwc_version == UBWC_3_0)
> > + value |= BIT(10);
> > +
> > + if (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)
> > +{
> > + u32 value = (ubwc_swizzle & 0x7) |
> > + (ubwc_static & 0x1) << 3 |
> > + (highest_bank_bit & 0x7) << 4 |
> > + (macrotile_mode & 0x1) << 12;
> > +
> > + writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
> > +
> > + if (ubwc_version == UBWC_3_0) {
> > + writel_relaxed(1, msm_mdss->mmio + UBWC_CTRL_2);
> > + writel_relaxed(0, msm_mdss->mmio + UBWC_PREDICTION_MODE);
> > + } else {
> > + writel_relaxed(2, msm_mdss->mmio + UBWC_CTRL_2);
> > + writel_relaxed(1, msm_mdss->mmio + UBWC_PREDICTION_MODE);
> > + }
> > +}
> > +
>
> Is it possible to unify the above functions by having the internal
> ubwc_version checks?
Note, it's not the ubwc_version, it is the ubwc_dec_hw_version. And
also different functions take different sets of arguments.
> It seems like msm_mdss_setup_ubwc_dec_xxx can keep growing.
>
> I have not looked into each bit programming but from the top level so
> feel free to correct if wrong but it seems both do write UBWC_STATIC
> (different values based on different UBWC versions) and write some extra
> registers based on version
This is what both the current code and the downstream do. See
https://github.com/MiCode/Xiaomi_Kernel_OpenSource/blob/zeus-s-oss/techpack/display-drivers/msm/sde/sde_hw_top.c#L312
>
> > static int msm_mdss_enable(struct msm_mdss *msm_mdss)
> > {
> > int ret;
> > + u32 hw_rev;
> >
> > ret = clk_bulk_prepare_enable(msm_mdss->num_clocks, msm_mdss->clocks);
> > if (ret) {
> > @@ -149,26 +204,34 @@ static int msm_mdss_enable(struct msm_mdss *msm_mdss)
> > if (msm_mdss->is_mdp5)
> > return 0;
> >
> > + hw_rev = readl_relaxed(msm_mdss->mmio + HW_REV);
> > + dev_info(msm_mdss->dev, "HW_REV: 0x%x\n", hw_rev);
> > + dev_info(msm_mdss->dev, "UBWC_DEC_HW_VERSION: 0x%x\n",
> > + readl_relaxed(msm_mdss->mmio + UBWC_DEC_HW_VERSION));
>
> we are already printing the HW version here
>
> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c#L1096
>
> Do you want to remove that print then? May be. Let me take a look.
[skipped]
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Freedreno] [PATCH] drm/msm: less magic numbers in msm_mdss_enable
2022-06-01 9:46 ` Dmitry Baryshkov
@ 2022-06-01 17:37 ` Abhinav Kumar
-1 siblings, 0 replies; 32+ messages in thread
From: Abhinav Kumar @ 2022-06-01 17:37 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: freedreno, David Airlie, linux-arm-msm, dri-devel,
Bjorn Andersson, Rob Clark, Daniel Vetter, Stephen Boyd,
Sean Paul
On 6/1/2022 2:46 AM, Dmitry Baryshkov wrote:
> On Wed, 1 Jun 2022 at 01:01, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>> On 5/31/2022 5:18 AM, Dmitry Baryshkov wrote:
>>> Replace magic register writes in msm_mdss_enable() with version that
>>> contains less magic and more variable names that can be traced back to
>>> the dpu_hw_catalog or the downstream dtsi files.
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>> drivers/gpu/drm/msm/msm_mdss.c | 79 ++++++++++++++++++++++++++++++----
>>> 1 file changed, 71 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
>>> index 0454a571adf7..2a48263cd1b5 100644
>>> --- a/drivers/gpu/drm/msm/msm_mdss.c
>>> +++ b/drivers/gpu/drm/msm/msm_mdss.c
>>> @@ -21,6 +21,7 @@
>>> #define HW_REV 0x0
>>> #define HW_INTR_STATUS 0x0010
>>>
>>> +#define UBWC_DEC_HW_VERSION 0x58
>>> #define UBWC_STATIC 0x144
>>> #define UBWC_CTRL_2 0x150
>>> #define UBWC_PREDICTION_MODE 0x154
>>> @@ -132,9 +133,63 @@ static int _msm_mdss_irq_domain_add(struct msm_mdss *msm_mdss)
>>> return 0;
>>> }
>>>
>>> +#define UBWC_1_0 0x10000000
>>> +#define UBWC_2_0 0x20000000
>>> +#define UBWC_3_0 0x30000000
>>> +#define UBWC_4_0 0x40000000
>>> +
>>> +static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss,
>>> + u32 ubwc_static)
>>> +{
>>> + writel_relaxed(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)
>>> +{
>>> + u32 value = (ubwc_swizzle & 0x1) |
>>> + (highest_bank_bit & 0x3) << 4 |
>>> + (macrotile_mode & 0x1) << 12;
>>> +
>>> + if (ubwc_version == UBWC_3_0)
>>> + value |= BIT(10);
>>> +
>>> + if (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)
>>> +{
>>> + u32 value = (ubwc_swizzle & 0x7) |
>>> + (ubwc_static & 0x1) << 3 |
>>> + (highest_bank_bit & 0x7) << 4 |
>>> + (macrotile_mode & 0x1) << 12;
>>> +
>>> + writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
>>> +
>>> + if (ubwc_version == UBWC_3_0) {
>>> + writel_relaxed(1, msm_mdss->mmio + UBWC_CTRL_2);
>>> + writel_relaxed(0, msm_mdss->mmio + UBWC_PREDICTION_MODE);
>>> + } else {
>>> + writel_relaxed(2, msm_mdss->mmio + UBWC_CTRL_2);
>>> + writel_relaxed(1, msm_mdss->mmio + UBWC_PREDICTION_MODE);
>>> + }
>>> +}
>>> +
>>
>> Is it possible to unify the above functions by having the internal
>> ubwc_version checks?
>
> Note, it's not the ubwc_version, it is the ubwc_dec_hw_version. And
> also different functions take different sets of arguments.
>
>> It seems like msm_mdss_setup_ubwc_dec_xxx can keep growing.
>>
>> I have not looked into each bit programming but from the top level so
>> feel free to correct if wrong but it seems both do write UBWC_STATIC
>> (different values based on different UBWC versions) and write some extra
>> registers based on version
>
> This is what both the current code and the downstream do. See
> https://github.com/MiCode/Xiaomi_Kernel_OpenSource/blob/zeus-s-oss/techpack/display-drivers/msm/sde/sde_hw_top.c#L312
>
Thanks for pointing to the downstream method for this,
This is exactly what i was also suggesting to do when I mentioned
unifying the above functions.
So instead of having a separate function for each version why not handle
all the versions in the same function like what the link you have shown
does.
>>
>>> static int msm_mdss_enable(struct msm_mdss *msm_mdss)
>>> {
>>> int ret;
>>> + u32 hw_rev;
>>>
>>> ret = clk_bulk_prepare_enable(msm_mdss->num_clocks, msm_mdss->clocks);
>>> if (ret) {
>>> @@ -149,26 +204,34 @@ static int msm_mdss_enable(struct msm_mdss *msm_mdss)
>>> if (msm_mdss->is_mdp5)
>>> return 0;
>>>
>>> + hw_rev = readl_relaxed(msm_mdss->mmio + HW_REV);
>>> + dev_info(msm_mdss->dev, "HW_REV: 0x%x\n", hw_rev);
>>> + dev_info(msm_mdss->dev, "UBWC_DEC_HW_VERSION: 0x%x\n",
>>> + readl_relaxed(msm_mdss->mmio + UBWC_DEC_HW_VERSION));
>>
>> we are already printing the HW version here
>>
>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c#L1096
>>
>> Do you want to remove that print then? May be. Let me take a look.
>
> [skipped]
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Freedreno] [PATCH] drm/msm: less magic numbers in msm_mdss_enable
@ 2022-06-01 17:37 ` Abhinav Kumar
0 siblings, 0 replies; 32+ messages in thread
From: Abhinav Kumar @ 2022-06-01 17:37 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Sean Paul, David Airlie, linux-arm-msm, dri-devel,
Bjorn Andersson, Stephen Boyd, freedreno
On 6/1/2022 2:46 AM, Dmitry Baryshkov wrote:
> On Wed, 1 Jun 2022 at 01:01, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>> On 5/31/2022 5:18 AM, Dmitry Baryshkov wrote:
>>> Replace magic register writes in msm_mdss_enable() with version that
>>> contains less magic and more variable names that can be traced back to
>>> the dpu_hw_catalog or the downstream dtsi files.
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>> drivers/gpu/drm/msm/msm_mdss.c | 79 ++++++++++++++++++++++++++++++----
>>> 1 file changed, 71 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
>>> index 0454a571adf7..2a48263cd1b5 100644
>>> --- a/drivers/gpu/drm/msm/msm_mdss.c
>>> +++ b/drivers/gpu/drm/msm/msm_mdss.c
>>> @@ -21,6 +21,7 @@
>>> #define HW_REV 0x0
>>> #define HW_INTR_STATUS 0x0010
>>>
>>> +#define UBWC_DEC_HW_VERSION 0x58
>>> #define UBWC_STATIC 0x144
>>> #define UBWC_CTRL_2 0x150
>>> #define UBWC_PREDICTION_MODE 0x154
>>> @@ -132,9 +133,63 @@ static int _msm_mdss_irq_domain_add(struct msm_mdss *msm_mdss)
>>> return 0;
>>> }
>>>
>>> +#define UBWC_1_0 0x10000000
>>> +#define UBWC_2_0 0x20000000
>>> +#define UBWC_3_0 0x30000000
>>> +#define UBWC_4_0 0x40000000
>>> +
>>> +static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss,
>>> + u32 ubwc_static)
>>> +{
>>> + writel_relaxed(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)
>>> +{
>>> + u32 value = (ubwc_swizzle & 0x1) |
>>> + (highest_bank_bit & 0x3) << 4 |
>>> + (macrotile_mode & 0x1) << 12;
>>> +
>>> + if (ubwc_version == UBWC_3_0)
>>> + value |= BIT(10);
>>> +
>>> + if (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)
>>> +{
>>> + u32 value = (ubwc_swizzle & 0x7) |
>>> + (ubwc_static & 0x1) << 3 |
>>> + (highest_bank_bit & 0x7) << 4 |
>>> + (macrotile_mode & 0x1) << 12;
>>> +
>>> + writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
>>> +
>>> + if (ubwc_version == UBWC_3_0) {
>>> + writel_relaxed(1, msm_mdss->mmio + UBWC_CTRL_2);
>>> + writel_relaxed(0, msm_mdss->mmio + UBWC_PREDICTION_MODE);
>>> + } else {
>>> + writel_relaxed(2, msm_mdss->mmio + UBWC_CTRL_2);
>>> + writel_relaxed(1, msm_mdss->mmio + UBWC_PREDICTION_MODE);
>>> + }
>>> +}
>>> +
>>
>> Is it possible to unify the above functions by having the internal
>> ubwc_version checks?
>
> Note, it's not the ubwc_version, it is the ubwc_dec_hw_version. And
> also different functions take different sets of arguments.
>
>> It seems like msm_mdss_setup_ubwc_dec_xxx can keep growing.
>>
>> I have not looked into each bit programming but from the top level so
>> feel free to correct if wrong but it seems both do write UBWC_STATIC
>> (different values based on different UBWC versions) and write some extra
>> registers based on version
>
> This is what both the current code and the downstream do. See
> https://github.com/MiCode/Xiaomi_Kernel_OpenSource/blob/zeus-s-oss/techpack/display-drivers/msm/sde/sde_hw_top.c#L312
>
Thanks for pointing to the downstream method for this,
This is exactly what i was also suggesting to do when I mentioned
unifying the above functions.
So instead of having a separate function for each version why not handle
all the versions in the same function like what the link you have shown
does.
>>
>>> static int msm_mdss_enable(struct msm_mdss *msm_mdss)
>>> {
>>> int ret;
>>> + u32 hw_rev;
>>>
>>> ret = clk_bulk_prepare_enable(msm_mdss->num_clocks, msm_mdss->clocks);
>>> if (ret) {
>>> @@ -149,26 +204,34 @@ static int msm_mdss_enable(struct msm_mdss *msm_mdss)
>>> if (msm_mdss->is_mdp5)
>>> return 0;
>>>
>>> + hw_rev = readl_relaxed(msm_mdss->mmio + HW_REV);
>>> + dev_info(msm_mdss->dev, "HW_REV: 0x%x\n", hw_rev);
>>> + dev_info(msm_mdss->dev, "UBWC_DEC_HW_VERSION: 0x%x\n",
>>> + readl_relaxed(msm_mdss->mmio + UBWC_DEC_HW_VERSION));
>>
>> we are already printing the HW version here
>>
>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c#L1096
>>
>> Do you want to remove that print then? May be. Let me take a look.
>
> [skipped]
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Freedreno] [PATCH] drm/msm: less magic numbers in msm_mdss_enable
2022-06-01 17:37 ` Abhinav Kumar
@ 2022-06-01 20:04 ` Dmitry Baryshkov
-1 siblings, 0 replies; 32+ messages in thread
From: Dmitry Baryshkov @ 2022-06-01 20:04 UTC (permalink / raw)
To: Abhinav Kumar
Cc: Sean Paul, David Airlie, linux-arm-msm, dri-devel,
Bjorn Andersson, Stephen Boyd, freedreno
On Wed, 1 Jun 2022 at 20:38, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 6/1/2022 2:46 AM, Dmitry Baryshkov wrote:
> > On Wed, 1 Jun 2022 at 01:01, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >> On 5/31/2022 5:18 AM, Dmitry Baryshkov wrote:
> >>> Replace magic register writes in msm_mdss_enable() with version that
> >>> contains less magic and more variable names that can be traced back to
> >>> the dpu_hw_catalog or the downstream dtsi files.
> >>>
> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >>> ---
> >>> drivers/gpu/drm/msm/msm_mdss.c | 79 ++++++++++++++++++++++++++++++----
> >>> 1 file changed, 71 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
> >>> index 0454a571adf7..2a48263cd1b5 100644
> >>> --- a/drivers/gpu/drm/msm/msm_mdss.c
> >>> +++ b/drivers/gpu/drm/msm/msm_mdss.c
> >>> @@ -21,6 +21,7 @@
> >>> #define HW_REV 0x0
> >>> #define HW_INTR_STATUS 0x0010
> >>>
> >>> +#define UBWC_DEC_HW_VERSION 0x58
> >>> #define UBWC_STATIC 0x144
> >>> #define UBWC_CTRL_2 0x150
> >>> #define UBWC_PREDICTION_MODE 0x154
> >>> @@ -132,9 +133,63 @@ static int _msm_mdss_irq_domain_add(struct msm_mdss *msm_mdss)
> >>> return 0;
> >>> }
> >>>
> >>> +#define UBWC_1_0 0x10000000
> >>> +#define UBWC_2_0 0x20000000
> >>> +#define UBWC_3_0 0x30000000
> >>> +#define UBWC_4_0 0x40000000
> >>> +
> >>> +static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss,
> >>> + u32 ubwc_static)
> >>> +{
> >>> + writel_relaxed(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)
> >>> +{
> >>> + u32 value = (ubwc_swizzle & 0x1) |
> >>> + (highest_bank_bit & 0x3) << 4 |
> >>> + (macrotile_mode & 0x1) << 12;
> >>> +
> >>> + if (ubwc_version == UBWC_3_0)
> >>> + value |= BIT(10);
> >>> +
> >>> + if (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)
> >>> +{
> >>> + u32 value = (ubwc_swizzle & 0x7) |
> >>> + (ubwc_static & 0x1) << 3 |
> >>> + (highest_bank_bit & 0x7) << 4 |
> >>> + (macrotile_mode & 0x1) << 12;
> >>> +
> >>> + writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
> >>> +
> >>> + if (ubwc_version == UBWC_3_0) {
> >>> + writel_relaxed(1, msm_mdss->mmio + UBWC_CTRL_2);
> >>> + writel_relaxed(0, msm_mdss->mmio + UBWC_PREDICTION_MODE);
> >>> + } else {
> >>> + writel_relaxed(2, msm_mdss->mmio + UBWC_CTRL_2);
> >>> + writel_relaxed(1, msm_mdss->mmio + UBWC_PREDICTION_MODE);
> >>> + }
> >>> +}
> >>> +
> >>
> >> Is it possible to unify the above functions by having the internal
> >> ubwc_version checks?
> >
> > Note, it's not the ubwc_version, it is the ubwc_dec_hw_version. And
> > also different functions take different sets of arguments.
> >
> >> It seems like msm_mdss_setup_ubwc_dec_xxx can keep growing.
> >>
> >> I have not looked into each bit programming but from the top level so
> >> feel free to correct if wrong but it seems both do write UBWC_STATIC
> >> (different values based on different UBWC versions) and write some extra
> >> registers based on version
> >
> > This is what both the current code and the downstream do. See
> > https://github.com/MiCode/Xiaomi_Kernel_OpenSource/blob/zeus-s-oss/techpack/display-drivers/msm/sde/sde_hw_top.c#L312
> >
>
> Thanks for pointing to the downstream method for this,
>
> This is exactly what i was also suggesting to do when I mentioned
> unifying the above functions.
>
> So instead of having a separate function for each version why not handle
> all the versions in the same function like what the link you have shown
> does.
I wouldn't like that. The downstream uses hw_catalog to pass all
possible parameters. We do not, so we'd have a whole set of artificial
values.
>
> >>
> >>> static int msm_mdss_enable(struct msm_mdss *msm_mdss)
> >>> {
> >>> int ret;
> >>> + u32 hw_rev;
> >>>
> >>> ret = clk_bulk_prepare_enable(msm_mdss->num_clocks, msm_mdss->clocks);
> >>> if (ret) {
> >>> @@ -149,26 +204,34 @@ static int msm_mdss_enable(struct msm_mdss *msm_mdss)
> >>> if (msm_mdss->is_mdp5)
> >>> return 0;
> >>>
> >>> + hw_rev = readl_relaxed(msm_mdss->mmio + HW_REV);
> >>> + dev_info(msm_mdss->dev, "HW_REV: 0x%x\n", hw_rev);
> >>> + dev_info(msm_mdss->dev, "UBWC_DEC_HW_VERSION: 0x%x\n",
> >>> + readl_relaxed(msm_mdss->mmio + UBWC_DEC_HW_VERSION));
> >>
> >> we are already printing the HW version here
> >>
> >> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c#L1096
> >>
> >> Do you want to remove that print then? May be. Let me take a look.
> >
> > [skipped]
> >
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Freedreno] [PATCH] drm/msm: less magic numbers in msm_mdss_enable
@ 2022-06-01 20:04 ` Dmitry Baryshkov
0 siblings, 0 replies; 32+ messages in thread
From: Dmitry Baryshkov @ 2022-06-01 20:04 UTC (permalink / raw)
To: Abhinav Kumar
Cc: freedreno, David Airlie, linux-arm-msm, dri-devel,
Bjorn Andersson, Rob Clark, Daniel Vetter, Stephen Boyd,
Sean Paul
On Wed, 1 Jun 2022 at 20:38, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 6/1/2022 2:46 AM, Dmitry Baryshkov wrote:
> > On Wed, 1 Jun 2022 at 01:01, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >> On 5/31/2022 5:18 AM, Dmitry Baryshkov wrote:
> >>> Replace magic register writes in msm_mdss_enable() with version that
> >>> contains less magic and more variable names that can be traced back to
> >>> the dpu_hw_catalog or the downstream dtsi files.
> >>>
> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >>> ---
> >>> drivers/gpu/drm/msm/msm_mdss.c | 79 ++++++++++++++++++++++++++++++----
> >>> 1 file changed, 71 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
> >>> index 0454a571adf7..2a48263cd1b5 100644
> >>> --- a/drivers/gpu/drm/msm/msm_mdss.c
> >>> +++ b/drivers/gpu/drm/msm/msm_mdss.c
> >>> @@ -21,6 +21,7 @@
> >>> #define HW_REV 0x0
> >>> #define HW_INTR_STATUS 0x0010
> >>>
> >>> +#define UBWC_DEC_HW_VERSION 0x58
> >>> #define UBWC_STATIC 0x144
> >>> #define UBWC_CTRL_2 0x150
> >>> #define UBWC_PREDICTION_MODE 0x154
> >>> @@ -132,9 +133,63 @@ static int _msm_mdss_irq_domain_add(struct msm_mdss *msm_mdss)
> >>> return 0;
> >>> }
> >>>
> >>> +#define UBWC_1_0 0x10000000
> >>> +#define UBWC_2_0 0x20000000
> >>> +#define UBWC_3_0 0x30000000
> >>> +#define UBWC_4_0 0x40000000
> >>> +
> >>> +static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss,
> >>> + u32 ubwc_static)
> >>> +{
> >>> + writel_relaxed(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)
> >>> +{
> >>> + u32 value = (ubwc_swizzle & 0x1) |
> >>> + (highest_bank_bit & 0x3) << 4 |
> >>> + (macrotile_mode & 0x1) << 12;
> >>> +
> >>> + if (ubwc_version == UBWC_3_0)
> >>> + value |= BIT(10);
> >>> +
> >>> + if (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)
> >>> +{
> >>> + u32 value = (ubwc_swizzle & 0x7) |
> >>> + (ubwc_static & 0x1) << 3 |
> >>> + (highest_bank_bit & 0x7) << 4 |
> >>> + (macrotile_mode & 0x1) << 12;
> >>> +
> >>> + writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
> >>> +
> >>> + if (ubwc_version == UBWC_3_0) {
> >>> + writel_relaxed(1, msm_mdss->mmio + UBWC_CTRL_2);
> >>> + writel_relaxed(0, msm_mdss->mmio + UBWC_PREDICTION_MODE);
> >>> + } else {
> >>> + writel_relaxed(2, msm_mdss->mmio + UBWC_CTRL_2);
> >>> + writel_relaxed(1, msm_mdss->mmio + UBWC_PREDICTION_MODE);
> >>> + }
> >>> +}
> >>> +
> >>
> >> Is it possible to unify the above functions by having the internal
> >> ubwc_version checks?
> >
> > Note, it's not the ubwc_version, it is the ubwc_dec_hw_version. And
> > also different functions take different sets of arguments.
> >
> >> It seems like msm_mdss_setup_ubwc_dec_xxx can keep growing.
> >>
> >> I have not looked into each bit programming but from the top level so
> >> feel free to correct if wrong but it seems both do write UBWC_STATIC
> >> (different values based on different UBWC versions) and write some extra
> >> registers based on version
> >
> > This is what both the current code and the downstream do. See
> > https://github.com/MiCode/Xiaomi_Kernel_OpenSource/blob/zeus-s-oss/techpack/display-drivers/msm/sde/sde_hw_top.c#L312
> >
>
> Thanks for pointing to the downstream method for this,
>
> This is exactly what i was also suggesting to do when I mentioned
> unifying the above functions.
>
> So instead of having a separate function for each version why not handle
> all the versions in the same function like what the link you have shown
> does.
I wouldn't like that. The downstream uses hw_catalog to pass all
possible parameters. We do not, so we'd have a whole set of artificial
values.
>
> >>
> >>> static int msm_mdss_enable(struct msm_mdss *msm_mdss)
> >>> {
> >>> int ret;
> >>> + u32 hw_rev;
> >>>
> >>> ret = clk_bulk_prepare_enable(msm_mdss->num_clocks, msm_mdss->clocks);
> >>> if (ret) {
> >>> @@ -149,26 +204,34 @@ static int msm_mdss_enable(struct msm_mdss *msm_mdss)
> >>> if (msm_mdss->is_mdp5)
> >>> return 0;
> >>>
> >>> + hw_rev = readl_relaxed(msm_mdss->mmio + HW_REV);
> >>> + dev_info(msm_mdss->dev, "HW_REV: 0x%x\n", hw_rev);
> >>> + dev_info(msm_mdss->dev, "UBWC_DEC_HW_VERSION: 0x%x\n",
> >>> + readl_relaxed(msm_mdss->mmio + UBWC_DEC_HW_VERSION));
> >>
> >> we are already printing the HW version here
> >>
> >> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c#L1096
> >>
> >> Do you want to remove that print then? May be. Let me take a look.
> >
> > [skipped]
> >
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Freedreno] [PATCH] drm/msm: less magic numbers in msm_mdss_enable
2022-06-01 20:04 ` Dmitry Baryshkov
@ 2022-06-02 18:18 ` Abhinav Kumar
-1 siblings, 0 replies; 32+ messages in thread
From: Abhinav Kumar @ 2022-06-02 18:18 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Sean Paul, David Airlie, linux-arm-msm, dri-devel,
Bjorn Andersson, Stephen Boyd, freedreno
On 6/1/2022 1:04 PM, Dmitry Baryshkov wrote:
> On Wed, 1 Jun 2022 at 20:38, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 6/1/2022 2:46 AM, Dmitry Baryshkov wrote:
>>> On Wed, 1 Jun 2022 at 01:01, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>>> On 5/31/2022 5:18 AM, Dmitry Baryshkov wrote:
>>>>> Replace magic register writes in msm_mdss_enable() with version that
>>>>> contains less magic and more variable names that can be traced back to
>>>>> the dpu_hw_catalog or the downstream dtsi files.
>>>>>
>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>> ---
>>>>> drivers/gpu/drm/msm/msm_mdss.c | 79 ++++++++++++++++++++++++++++++----
>>>>> 1 file changed, 71 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
>>>>> index 0454a571adf7..2a48263cd1b5 100644
>>>>> --- a/drivers/gpu/drm/msm/msm_mdss.c
>>>>> +++ b/drivers/gpu/drm/msm/msm_mdss.c
>>>>> @@ -21,6 +21,7 @@
>>>>> #define HW_REV 0x0
>>>>> #define HW_INTR_STATUS 0x0010
>>>>>
>>>>> +#define UBWC_DEC_HW_VERSION 0x58
>>>>> #define UBWC_STATIC 0x144
>>>>> #define UBWC_CTRL_2 0x150
>>>>> #define UBWC_PREDICTION_MODE 0x154
>>>>> @@ -132,9 +133,63 @@ static int _msm_mdss_irq_domain_add(struct msm_mdss *msm_mdss)
>>>>> return 0;
>>>>> }
>>>>>
>>>>> +#define UBWC_1_0 0x10000000
>>>>> +#define UBWC_2_0 0x20000000
>>>>> +#define UBWC_3_0 0x30000000
>>>>> +#define UBWC_4_0 0x40000000
>>>>> +
>>>>> +static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss,
>>>>> + u32 ubwc_static)
>>>>> +{
>>>>> + writel_relaxed(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)
>>>>> +{
>>>>> + u32 value = (ubwc_swizzle & 0x1) |
>>>>> + (highest_bank_bit & 0x3) << 4 |
>>>>> + (macrotile_mode & 0x1) << 12;
>>>>> +
>>>>> + if (ubwc_version == UBWC_3_0)
>>>>> + value |= BIT(10);
>>>>> +
>>>>> + if (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)
>>>>> +{
>>>>> + u32 value = (ubwc_swizzle & 0x7) |
>>>>> + (ubwc_static & 0x1) << 3 |
>>>>> + (highest_bank_bit & 0x7) << 4 |
>>>>> + (macrotile_mode & 0x1) << 12;
>>>>> +
>>>>> + writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
>>>>> +
>>>>> + if (ubwc_version == UBWC_3_0) {
>>>>> + writel_relaxed(1, msm_mdss->mmio + UBWC_CTRL_2);
>>>>> + writel_relaxed(0, msm_mdss->mmio + UBWC_PREDICTION_MODE);
>>>>> + } else {
>>>>> + writel_relaxed(2, msm_mdss->mmio + UBWC_CTRL_2);
>>>>> + writel_relaxed(1, msm_mdss->mmio + UBWC_PREDICTION_MODE);
>>>>> + }
>>>>> +}
>>>>> +
>>>>
>>>> Is it possible to unify the above functions by having the internal
>>>> ubwc_version checks?
>>>
>>> Note, it's not the ubwc_version, it is the ubwc_dec_hw_version. And
>>> also different functions take different sets of arguments.
>>>
>>>> It seems like msm_mdss_setup_ubwc_dec_xxx can keep growing.
>>>>
>>>> I have not looked into each bit programming but from the top level so
>>>> feel free to correct if wrong but it seems both do write UBWC_STATIC
>>>> (different values based on different UBWC versions) and write some extra
>>>> registers based on version
>>>
>>> This is what both the current code and the downstream do. See
>>> https://github.com/MiCode/Xiaomi_Kernel_OpenSource/blob/zeus-s-oss/techpack/display-drivers/msm/sde/sde_hw_top.c#L312
>>>
>>
>> Thanks for pointing to the downstream method for this,
>>
>> This is exactly what i was also suggesting to do when I mentioned
>> unifying the above functions.
>>
>> So instead of having a separate function for each version why not handle
>> all the versions in the same function like what the link you have shown
>> does.
>
> I wouldn't like that. The downstream uses hw_catalog to pass all
> possible parameters. We do not, so we'd have a whole set of artificial
> values.
>
Now that you brought that up, why cannot even upstream dpu start using
catalog for ubwc settings?
/* struct dpu_mdp_cfg : MDP TOP-BLK instance info
* @id: index identifying this block
* @base: register base offset to mdss
* @features bit mask identifying sub-blocks/features
* @highest_bank_bit: UBWC parameter
* @ubwc_static: ubwc static configuration
* @ubwc_swizzle: ubwc default swizzle setting
* @clk_ctrls clock control register definition
*/
struct dpu_mdp_cfg {
DPU_HW_BLK_INFO;
u32 highest_bank_bit;
u32 ubwc_swizzle;
struct dpu_clk_ctrl_reg clk_ctrls[DPU_CLK_CTRL_MAX];
};
We already do seem to have a couple of parameters. have to add the others.
That way the number of functions wont keep growing.
>>
>>>>
>>>>> static int msm_mdss_enable(struct msm_mdss *msm_mdss)
>>>>> {
>>>>> int ret;
>>>>> + u32 hw_rev;
>>>>>
>>>>> ret = clk_bulk_prepare_enable(msm_mdss->num_clocks, msm_mdss->clocks);
>>>>> if (ret) {
>>>>> @@ -149,26 +204,34 @@ static int msm_mdss_enable(struct msm_mdss *msm_mdss)
>>>>> if (msm_mdss->is_mdp5)
>>>>> return 0;
>>>>>
>>>>> + hw_rev = readl_relaxed(msm_mdss->mmio + HW_REV);
>>>>> + dev_info(msm_mdss->dev, "HW_REV: 0x%x\n", hw_rev);
>>>>> + dev_info(msm_mdss->dev, "UBWC_DEC_HW_VERSION: 0x%x\n",
>>>>> + readl_relaxed(msm_mdss->mmio + UBWC_DEC_HW_VERSION));
>>>>
>>>> we are already printing the HW version here
>>>>
>>>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c#L1096
>>>>
>>>> Do you want to remove that print then? May be. Let me take a look.
>>>
>>> [skipped]
>>>
>
>
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Freedreno] [PATCH] drm/msm: less magic numbers in msm_mdss_enable
@ 2022-06-02 18:18 ` Abhinav Kumar
0 siblings, 0 replies; 32+ messages in thread
From: Abhinav Kumar @ 2022-06-02 18:18 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: freedreno, David Airlie, linux-arm-msm, dri-devel, Stephen Boyd,
Bjorn Andersson, Sean Paul
On 6/1/2022 1:04 PM, Dmitry Baryshkov wrote:
> On Wed, 1 Jun 2022 at 20:38, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 6/1/2022 2:46 AM, Dmitry Baryshkov wrote:
>>> On Wed, 1 Jun 2022 at 01:01, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>>> On 5/31/2022 5:18 AM, Dmitry Baryshkov wrote:
>>>>> Replace magic register writes in msm_mdss_enable() with version that
>>>>> contains less magic and more variable names that can be traced back to
>>>>> the dpu_hw_catalog or the downstream dtsi files.
>>>>>
>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>> ---
>>>>> drivers/gpu/drm/msm/msm_mdss.c | 79 ++++++++++++++++++++++++++++++----
>>>>> 1 file changed, 71 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
>>>>> index 0454a571adf7..2a48263cd1b5 100644
>>>>> --- a/drivers/gpu/drm/msm/msm_mdss.c
>>>>> +++ b/drivers/gpu/drm/msm/msm_mdss.c
>>>>> @@ -21,6 +21,7 @@
>>>>> #define HW_REV 0x0
>>>>> #define HW_INTR_STATUS 0x0010
>>>>>
>>>>> +#define UBWC_DEC_HW_VERSION 0x58
>>>>> #define UBWC_STATIC 0x144
>>>>> #define UBWC_CTRL_2 0x150
>>>>> #define UBWC_PREDICTION_MODE 0x154
>>>>> @@ -132,9 +133,63 @@ static int _msm_mdss_irq_domain_add(struct msm_mdss *msm_mdss)
>>>>> return 0;
>>>>> }
>>>>>
>>>>> +#define UBWC_1_0 0x10000000
>>>>> +#define UBWC_2_0 0x20000000
>>>>> +#define UBWC_3_0 0x30000000
>>>>> +#define UBWC_4_0 0x40000000
>>>>> +
>>>>> +static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss,
>>>>> + u32 ubwc_static)
>>>>> +{
>>>>> + writel_relaxed(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)
>>>>> +{
>>>>> + u32 value = (ubwc_swizzle & 0x1) |
>>>>> + (highest_bank_bit & 0x3) << 4 |
>>>>> + (macrotile_mode & 0x1) << 12;
>>>>> +
>>>>> + if (ubwc_version == UBWC_3_0)
>>>>> + value |= BIT(10);
>>>>> +
>>>>> + if (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)
>>>>> +{
>>>>> + u32 value = (ubwc_swizzle & 0x7) |
>>>>> + (ubwc_static & 0x1) << 3 |
>>>>> + (highest_bank_bit & 0x7) << 4 |
>>>>> + (macrotile_mode & 0x1) << 12;
>>>>> +
>>>>> + writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
>>>>> +
>>>>> + if (ubwc_version == UBWC_3_0) {
>>>>> + writel_relaxed(1, msm_mdss->mmio + UBWC_CTRL_2);
>>>>> + writel_relaxed(0, msm_mdss->mmio + UBWC_PREDICTION_MODE);
>>>>> + } else {
>>>>> + writel_relaxed(2, msm_mdss->mmio + UBWC_CTRL_2);
>>>>> + writel_relaxed(1, msm_mdss->mmio + UBWC_PREDICTION_MODE);
>>>>> + }
>>>>> +}
>>>>> +
>>>>
>>>> Is it possible to unify the above functions by having the internal
>>>> ubwc_version checks?
>>>
>>> Note, it's not the ubwc_version, it is the ubwc_dec_hw_version. And
>>> also different functions take different sets of arguments.
>>>
>>>> It seems like msm_mdss_setup_ubwc_dec_xxx can keep growing.
>>>>
>>>> I have not looked into each bit programming but from the top level so
>>>> feel free to correct if wrong but it seems both do write UBWC_STATIC
>>>> (different values based on different UBWC versions) and write some extra
>>>> registers based on version
>>>
>>> This is what both the current code and the downstream do. See
>>> https://github.com/MiCode/Xiaomi_Kernel_OpenSource/blob/zeus-s-oss/techpack/display-drivers/msm/sde/sde_hw_top.c#L312
>>>
>>
>> Thanks for pointing to the downstream method for this,
>>
>> This is exactly what i was also suggesting to do when I mentioned
>> unifying the above functions.
>>
>> So instead of having a separate function for each version why not handle
>> all the versions in the same function like what the link you have shown
>> does.
>
> I wouldn't like that. The downstream uses hw_catalog to pass all
> possible parameters. We do not, so we'd have a whole set of artificial
> values.
>
Now that you brought that up, why cannot even upstream dpu start using
catalog for ubwc settings?
/* struct dpu_mdp_cfg : MDP TOP-BLK instance info
* @id: index identifying this block
* @base: register base offset to mdss
* @features bit mask identifying sub-blocks/features
* @highest_bank_bit: UBWC parameter
* @ubwc_static: ubwc static configuration
* @ubwc_swizzle: ubwc default swizzle setting
* @clk_ctrls clock control register definition
*/
struct dpu_mdp_cfg {
DPU_HW_BLK_INFO;
u32 highest_bank_bit;
u32 ubwc_swizzle;
struct dpu_clk_ctrl_reg clk_ctrls[DPU_CLK_CTRL_MAX];
};
We already do seem to have a couple of parameters. have to add the others.
That way the number of functions wont keep growing.
>>
>>>>
>>>>> static int msm_mdss_enable(struct msm_mdss *msm_mdss)
>>>>> {
>>>>> int ret;
>>>>> + u32 hw_rev;
>>>>>
>>>>> ret = clk_bulk_prepare_enable(msm_mdss->num_clocks, msm_mdss->clocks);
>>>>> if (ret) {
>>>>> @@ -149,26 +204,34 @@ static int msm_mdss_enable(struct msm_mdss *msm_mdss)
>>>>> if (msm_mdss->is_mdp5)
>>>>> return 0;
>>>>>
>>>>> + hw_rev = readl_relaxed(msm_mdss->mmio + HW_REV);
>>>>> + dev_info(msm_mdss->dev, "HW_REV: 0x%x\n", hw_rev);
>>>>> + dev_info(msm_mdss->dev, "UBWC_DEC_HW_VERSION: 0x%x\n",
>>>>> + readl_relaxed(msm_mdss->mmio + UBWC_DEC_HW_VERSION));
>>>>
>>>> we are already printing the HW version here
>>>>
>>>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c#L1096
>>>>
>>>> Do you want to remove that print then? May be. Let me take a look.
>>>
>>> [skipped]
>>>
>
>
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Freedreno] [PATCH] drm/msm: less magic numbers in msm_mdss_enable
2022-06-02 18:18 ` Abhinav Kumar
@ 2022-06-02 20:13 ` Dmitry Baryshkov
-1 siblings, 0 replies; 32+ messages in thread
From: Dmitry Baryshkov @ 2022-06-02 20:13 UTC (permalink / raw)
To: Abhinav Kumar
Cc: freedreno, David Airlie, linux-arm-msm, dri-devel, Stephen Boyd,
Bjorn Andersson, Sean Paul
On Thu, 2 Jun 2022 at 21:18, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 6/1/2022 1:04 PM, Dmitry Baryshkov wrote:
> > On Wed, 1 Jun 2022 at 20:38, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>
> >>
> >>
> >> On 6/1/2022 2:46 AM, Dmitry Baryshkov wrote:
> >>> On Wed, 1 Jun 2022 at 01:01, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>>> On 5/31/2022 5:18 AM, Dmitry Baryshkov wrote:
> >>>>> Replace magic register writes in msm_mdss_enable() with version that
> >>>>> contains less magic and more variable names that can be traced back to
> >>>>> the dpu_hw_catalog or the downstream dtsi files.
> >>>>>
> >>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >>>>> ---
> >>>>> drivers/gpu/drm/msm/msm_mdss.c | 79 ++++++++++++++++++++++++++++++----
> >>>>> 1 file changed, 71 insertions(+), 8 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
> >>>>> index 0454a571adf7..2a48263cd1b5 100644
> >>>>> --- a/drivers/gpu/drm/msm/msm_mdss.c
> >>>>> +++ b/drivers/gpu/drm/msm/msm_mdss.c
> >>>>> @@ -21,6 +21,7 @@
> >>>>> #define HW_REV 0x0
> >>>>> #define HW_INTR_STATUS 0x0010
> >>>>>
> >>>>> +#define UBWC_DEC_HW_VERSION 0x58
> >>>>> #define UBWC_STATIC 0x144
> >>>>> #define UBWC_CTRL_2 0x150
> >>>>> #define UBWC_PREDICTION_MODE 0x154
> >>>>> @@ -132,9 +133,63 @@ static int _msm_mdss_irq_domain_add(struct msm_mdss *msm_mdss)
> >>>>> return 0;
> >>>>> }
> >>>>>
> >>>>> +#define UBWC_1_0 0x10000000
> >>>>> +#define UBWC_2_0 0x20000000
> >>>>> +#define UBWC_3_0 0x30000000
> >>>>> +#define UBWC_4_0 0x40000000
> >>>>> +
> >>>>> +static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss,
> >>>>> + u32 ubwc_static)
> >>>>> +{
> >>>>> + writel_relaxed(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)
> >>>>> +{
> >>>>> + u32 value = (ubwc_swizzle & 0x1) |
> >>>>> + (highest_bank_bit & 0x3) << 4 |
> >>>>> + (macrotile_mode & 0x1) << 12;
> >>>>> +
> >>>>> + if (ubwc_version == UBWC_3_0)
> >>>>> + value |= BIT(10);
> >>>>> +
> >>>>> + if (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)
> >>>>> +{
> >>>>> + u32 value = (ubwc_swizzle & 0x7) |
> >>>>> + (ubwc_static & 0x1) << 3 |
> >>>>> + (highest_bank_bit & 0x7) << 4 |
> >>>>> + (macrotile_mode & 0x1) << 12;
> >>>>> +
> >>>>> + writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
> >>>>> +
> >>>>> + if (ubwc_version == UBWC_3_0) {
> >>>>> + writel_relaxed(1, msm_mdss->mmio + UBWC_CTRL_2);
> >>>>> + writel_relaxed(0, msm_mdss->mmio + UBWC_PREDICTION_MODE);
> >>>>> + } else {
> >>>>> + writel_relaxed(2, msm_mdss->mmio + UBWC_CTRL_2);
> >>>>> + writel_relaxed(1, msm_mdss->mmio + UBWC_PREDICTION_MODE);
> >>>>> + }
> >>>>> +}
> >>>>> +
> >>>>
> >>>> Is it possible to unify the above functions by having the internal
> >>>> ubwc_version checks?
> >>>
> >>> Note, it's not the ubwc_version, it is the ubwc_dec_hw_version. And
> >>> also different functions take different sets of arguments.
> >>>
> >>>> It seems like msm_mdss_setup_ubwc_dec_xxx can keep growing.
> >>>>
> >>>> I have not looked into each bit programming but from the top level so
> >>>> feel free to correct if wrong but it seems both do write UBWC_STATIC
> >>>> (different values based on different UBWC versions) and write some extra
> >>>> registers based on version
> >>>
> >>> This is what both the current code and the downstream do. See
> >>> https://github.com/MiCode/Xiaomi_Kernel_OpenSource/blob/zeus-s-oss/techpack/display-drivers/msm/sde/sde_hw_top.c#L312
> >>>
> >>
> >> Thanks for pointing to the downstream method for this,
> >>
> >> This is exactly what i was also suggesting to do when I mentioned
> >> unifying the above functions.
> >>
> >> So instead of having a separate function for each version why not handle
> >> all the versions in the same function like what the link you have shown
> >> does.
> >
> > I wouldn't like that. The downstream uses hw_catalog to pass all
> > possible parameters. We do not, so we'd have a whole set of artificial
> > values.
> >
>
> Now that you brought that up, why cannot even upstream dpu start using
> catalog for ubwc settings?
Because msm_mdss lives out of disp/dpu1. And using the disp/dpu1 for
it would be an inversion of dependencies.
I like the fact that msm_mdss is independent of mdp/dpu drivers and I
do not want to add such dependency.
>
> /* struct dpu_mdp_cfg : MDP TOP-BLK instance info
> * @id: index identifying this block
> * @base: register base offset to mdss
> * @features bit mask identifying sub-blocks/features
> * @highest_bank_bit: UBWC parameter
> * @ubwc_static: ubwc static configuration
> * @ubwc_swizzle: ubwc default swizzle setting
> * @clk_ctrls clock control register definition
> */
> struct dpu_mdp_cfg {
> DPU_HW_BLK_INFO;
> u32 highest_bank_bit;
> u32 ubwc_swizzle;
> struct dpu_clk_ctrl_reg clk_ctrls[DPU_CLK_CTRL_MAX];
> };
>
> We already do seem to have a couple of parameters. have to add the others.
>
> That way the number of functions wont keep growing.
>
> >>
> >>>>
> >>>>> static int msm_mdss_enable(struct msm_mdss *msm_mdss)
> >>>>> {
> >>>>> int ret;
> >>>>> + u32 hw_rev;
> >>>>>
> >>>>> ret = clk_bulk_prepare_enable(msm_mdss->num_clocks, msm_mdss->clocks);
> >>>>> if (ret) {
> >>>>> @@ -149,26 +204,34 @@ static int msm_mdss_enable(struct msm_mdss *msm_mdss)
> >>>>> if (msm_mdss->is_mdp5)
> >>>>> return 0;
> >>>>>
> >>>>> + hw_rev = readl_relaxed(msm_mdss->mmio + HW_REV);
> >>>>> + dev_info(msm_mdss->dev, "HW_REV: 0x%x\n", hw_rev);
> >>>>> + dev_info(msm_mdss->dev, "UBWC_DEC_HW_VERSION: 0x%x\n",
> >>>>> + readl_relaxed(msm_mdss->mmio + UBWC_DEC_HW_VERSION));
> >>>>
> >>>> we are already printing the HW version here
> >>>>
> >>>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c#L1096
> >>>>
> >>>> Do you want to remove that print then? May be. Let me take a look.
> >>>
> >>> [skipped]
> >>>
> >
> >
> >
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Freedreno] [PATCH] drm/msm: less magic numbers in msm_mdss_enable
@ 2022-06-02 20:13 ` Dmitry Baryshkov
0 siblings, 0 replies; 32+ messages in thread
From: Dmitry Baryshkov @ 2022-06-02 20:13 UTC (permalink / raw)
To: Abhinav Kumar
Cc: Sean Paul, David Airlie, linux-arm-msm, dri-devel,
Bjorn Andersson, Stephen Boyd, freedreno
On Thu, 2 Jun 2022 at 21:18, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 6/1/2022 1:04 PM, Dmitry Baryshkov wrote:
> > On Wed, 1 Jun 2022 at 20:38, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>
> >>
> >>
> >> On 6/1/2022 2:46 AM, Dmitry Baryshkov wrote:
> >>> On Wed, 1 Jun 2022 at 01:01, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>>> On 5/31/2022 5:18 AM, Dmitry Baryshkov wrote:
> >>>>> Replace magic register writes in msm_mdss_enable() with version that
> >>>>> contains less magic and more variable names that can be traced back to
> >>>>> the dpu_hw_catalog or the downstream dtsi files.
> >>>>>
> >>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >>>>> ---
> >>>>> drivers/gpu/drm/msm/msm_mdss.c | 79 ++++++++++++++++++++++++++++++----
> >>>>> 1 file changed, 71 insertions(+), 8 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
> >>>>> index 0454a571adf7..2a48263cd1b5 100644
> >>>>> --- a/drivers/gpu/drm/msm/msm_mdss.c
> >>>>> +++ b/drivers/gpu/drm/msm/msm_mdss.c
> >>>>> @@ -21,6 +21,7 @@
> >>>>> #define HW_REV 0x0
> >>>>> #define HW_INTR_STATUS 0x0010
> >>>>>
> >>>>> +#define UBWC_DEC_HW_VERSION 0x58
> >>>>> #define UBWC_STATIC 0x144
> >>>>> #define UBWC_CTRL_2 0x150
> >>>>> #define UBWC_PREDICTION_MODE 0x154
> >>>>> @@ -132,9 +133,63 @@ static int _msm_mdss_irq_domain_add(struct msm_mdss *msm_mdss)
> >>>>> return 0;
> >>>>> }
> >>>>>
> >>>>> +#define UBWC_1_0 0x10000000
> >>>>> +#define UBWC_2_0 0x20000000
> >>>>> +#define UBWC_3_0 0x30000000
> >>>>> +#define UBWC_4_0 0x40000000
> >>>>> +
> >>>>> +static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss,
> >>>>> + u32 ubwc_static)
> >>>>> +{
> >>>>> + writel_relaxed(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)
> >>>>> +{
> >>>>> + u32 value = (ubwc_swizzle & 0x1) |
> >>>>> + (highest_bank_bit & 0x3) << 4 |
> >>>>> + (macrotile_mode & 0x1) << 12;
> >>>>> +
> >>>>> + if (ubwc_version == UBWC_3_0)
> >>>>> + value |= BIT(10);
> >>>>> +
> >>>>> + if (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)
> >>>>> +{
> >>>>> + u32 value = (ubwc_swizzle & 0x7) |
> >>>>> + (ubwc_static & 0x1) << 3 |
> >>>>> + (highest_bank_bit & 0x7) << 4 |
> >>>>> + (macrotile_mode & 0x1) << 12;
> >>>>> +
> >>>>> + writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
> >>>>> +
> >>>>> + if (ubwc_version == UBWC_3_0) {
> >>>>> + writel_relaxed(1, msm_mdss->mmio + UBWC_CTRL_2);
> >>>>> + writel_relaxed(0, msm_mdss->mmio + UBWC_PREDICTION_MODE);
> >>>>> + } else {
> >>>>> + writel_relaxed(2, msm_mdss->mmio + UBWC_CTRL_2);
> >>>>> + writel_relaxed(1, msm_mdss->mmio + UBWC_PREDICTION_MODE);
> >>>>> + }
> >>>>> +}
> >>>>> +
> >>>>
> >>>> Is it possible to unify the above functions by having the internal
> >>>> ubwc_version checks?
> >>>
> >>> Note, it's not the ubwc_version, it is the ubwc_dec_hw_version. And
> >>> also different functions take different sets of arguments.
> >>>
> >>>> It seems like msm_mdss_setup_ubwc_dec_xxx can keep growing.
> >>>>
> >>>> I have not looked into each bit programming but from the top level so
> >>>> feel free to correct if wrong but it seems both do write UBWC_STATIC
> >>>> (different values based on different UBWC versions) and write some extra
> >>>> registers based on version
> >>>
> >>> This is what both the current code and the downstream do. See
> >>> https://github.com/MiCode/Xiaomi_Kernel_OpenSource/blob/zeus-s-oss/techpack/display-drivers/msm/sde/sde_hw_top.c#L312
> >>>
> >>
> >> Thanks for pointing to the downstream method for this,
> >>
> >> This is exactly what i was also suggesting to do when I mentioned
> >> unifying the above functions.
> >>
> >> So instead of having a separate function for each version why not handle
> >> all the versions in the same function like what the link you have shown
> >> does.
> >
> > I wouldn't like that. The downstream uses hw_catalog to pass all
> > possible parameters. We do not, so we'd have a whole set of artificial
> > values.
> >
>
> Now that you brought that up, why cannot even upstream dpu start using
> catalog for ubwc settings?
Because msm_mdss lives out of disp/dpu1. And using the disp/dpu1 for
it would be an inversion of dependencies.
I like the fact that msm_mdss is independent of mdp/dpu drivers and I
do not want to add such dependency.
>
> /* struct dpu_mdp_cfg : MDP TOP-BLK instance info
> * @id: index identifying this block
> * @base: register base offset to mdss
> * @features bit mask identifying sub-blocks/features
> * @highest_bank_bit: UBWC parameter
> * @ubwc_static: ubwc static configuration
> * @ubwc_swizzle: ubwc default swizzle setting
> * @clk_ctrls clock control register definition
> */
> struct dpu_mdp_cfg {
> DPU_HW_BLK_INFO;
> u32 highest_bank_bit;
> u32 ubwc_swizzle;
> struct dpu_clk_ctrl_reg clk_ctrls[DPU_CLK_CTRL_MAX];
> };
>
> We already do seem to have a couple of parameters. have to add the others.
>
> That way the number of functions wont keep growing.
>
> >>
> >>>>
> >>>>> static int msm_mdss_enable(struct msm_mdss *msm_mdss)
> >>>>> {
> >>>>> int ret;
> >>>>> + u32 hw_rev;
> >>>>>
> >>>>> ret = clk_bulk_prepare_enable(msm_mdss->num_clocks, msm_mdss->clocks);
> >>>>> if (ret) {
> >>>>> @@ -149,26 +204,34 @@ static int msm_mdss_enable(struct msm_mdss *msm_mdss)
> >>>>> if (msm_mdss->is_mdp5)
> >>>>> return 0;
> >>>>>
> >>>>> + hw_rev = readl_relaxed(msm_mdss->mmio + HW_REV);
> >>>>> + dev_info(msm_mdss->dev, "HW_REV: 0x%x\n", hw_rev);
> >>>>> + dev_info(msm_mdss->dev, "UBWC_DEC_HW_VERSION: 0x%x\n",
> >>>>> + readl_relaxed(msm_mdss->mmio + UBWC_DEC_HW_VERSION));
> >>>>
> >>>> we are already printing the HW version here
> >>>>
> >>>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c#L1096
> >>>>
> >>>> Do you want to remove that print then? May be. Let me take a look.
> >>>
> >>> [skipped]
> >>>
> >
> >
> >
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Freedreno] [PATCH] drm/msm: less magic numbers in msm_mdss_enable
2022-06-02 20:13 ` Dmitry Baryshkov
@ 2022-06-08 19:29 ` Abhinav Kumar
-1 siblings, 0 replies; 32+ messages in thread
From: Abhinav Kumar @ 2022-06-08 19:29 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: freedreno, David Airlie, linux-arm-msm, dri-devel, Stephen Boyd,
Bjorn Andersson, Sean Paul
On 6/2/2022 1:13 PM, Dmitry Baryshkov wrote:
> On Thu, 2 Jun 2022 at 21:18, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 6/1/2022 1:04 PM, Dmitry Baryshkov wrote:
>>> On Wed, 1 Jun 2022 at 20:38, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 6/1/2022 2:46 AM, Dmitry Baryshkov wrote:
>>>>> On Wed, 1 Jun 2022 at 01:01, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>>>>> On 5/31/2022 5:18 AM, Dmitry Baryshkov wrote:
>>>>>>> Replace magic register writes in msm_mdss_enable() with version that
>>>>>>> contains less magic and more variable names that can be traced back to
>>>>>>> the dpu_hw_catalog or the downstream dtsi files.
>>>>>>>
>>>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>>> ---
>>>>>>> drivers/gpu/drm/msm/msm_mdss.c | 79 ++++++++++++++++++++++++++++++----
>>>>>>> 1 file changed, 71 insertions(+), 8 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>> index 0454a571adf7..2a48263cd1b5 100644
>>>>>>> --- a/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>> +++ b/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>> @@ -21,6 +21,7 @@
>>>>>>> #define HW_REV 0x0
>>>>>>> #define HW_INTR_STATUS 0x0010
>>>>>>>
>>>>>>> +#define UBWC_DEC_HW_VERSION 0x58
>>>>>>> #define UBWC_STATIC 0x144
>>>>>>> #define UBWC_CTRL_2 0x150
>>>>>>> #define UBWC_PREDICTION_MODE 0x154
>>>>>>> @@ -132,9 +133,63 @@ static int _msm_mdss_irq_domain_add(struct msm_mdss *msm_mdss)
>>>>>>> return 0;
>>>>>>> }
>>>>>>>
>>>>>>> +#define UBWC_1_0 0x10000000
>>>>>>> +#define UBWC_2_0 0x20000000
>>>>>>> +#define UBWC_3_0 0x30000000
>>>>>>> +#define UBWC_4_0 0x40000000
>>>>>>> +
>>>>>>> +static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss,
>>>>>>> + u32 ubwc_static)
>>>>>>> +{
>>>>>>> + writel_relaxed(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)
>>>>>>> +{
>>>>>>> + u32 value = (ubwc_swizzle & 0x1) |
>>>>>>> + (highest_bank_bit & 0x3) << 4 |
>>>>>>> + (macrotile_mode & 0x1) << 12;
>>>>>>> +
>>>>>>> + if (ubwc_version == UBWC_3_0)
>>>>>>> + value |= BIT(10);
>>>>>>> +
>>>>>>> + if (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)
>>>>>>> +{
>>>>>>> + u32 value = (ubwc_swizzle & 0x7) |
>>>>>>> + (ubwc_static & 0x1) << 3 |
>>>>>>> + (highest_bank_bit & 0x7) << 4 |
>>>>>>> + (macrotile_mode & 0x1) << 12;
>>>>>>> +
>>>>>>> + writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
>>>>>>> +
>>>>>>> + if (ubwc_version == UBWC_3_0) {
>>>>>>> + writel_relaxed(1, msm_mdss->mmio + UBWC_CTRL_2);
>>>>>>> + writel_relaxed(0, msm_mdss->mmio + UBWC_PREDICTION_MODE);
>>>>>>> + } else {
>>>>>>> + writel_relaxed(2, msm_mdss->mmio + UBWC_CTRL_2);
>>>>>>> + writel_relaxed(1, msm_mdss->mmio + UBWC_PREDICTION_MODE);
>>>>>>> + }
>>>>>>> +}
>>>>>>> +
>>>>>>
>>>>>> Is it possible to unify the above functions by having the internal
>>>>>> ubwc_version checks?
>>>>>
>>>>> Note, it's not the ubwc_version, it is the ubwc_dec_hw_version. And
>>>>> also different functions take different sets of arguments.
>>>>>
>>>>>> It seems like msm_mdss_setup_ubwc_dec_xxx can keep growing.
>>>>>>
>>>>>> I have not looked into each bit programming but from the top level so
>>>>>> feel free to correct if wrong but it seems both do write UBWC_STATIC
>>>>>> (different values based on different UBWC versions) and write some extra
>>>>>> registers based on version
>>>>>
>>>>> This is what both the current code and the downstream do. See
>>>>> https://github.com/MiCode/Xiaomi_Kernel_OpenSource/blob/zeus-s-oss/techpack/display-drivers/msm/sde/sde_hw_top.c#L312
>>>>>
>>>>
>>>> Thanks for pointing to the downstream method for this,
>>>>
>>>> This is exactly what i was also suggesting to do when I mentioned
>>>> unifying the above functions.
>>>>
>>>> So instead of having a separate function for each version why not handle
>>>> all the versions in the same function like what the link you have shown
>>>> does.
>>>
>>> I wouldn't like that. The downstream uses hw_catalog to pass all
>>> possible parameters. We do not, so we'd have a whole set of artificial
>>> values.
>>>
>>
>> Now that you brought that up, why cannot even upstream dpu start using
>> catalog for ubwc settings?
>
> Because msm_mdss lives out of disp/dpu1. And using the disp/dpu1 for
> it would be an inversion of dependencies.
> I like the fact that msm_mdss is independent of mdp/dpu drivers and I
> do not want to add such dependency.
>
Ok, so I think this function itself is placed incorrectly. It should not
be in msm_mdss.c and should in the DPU folder.
This check tells me that this will not be executed for mdp5 devices.
/*
* 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)
return 0;
In that case, what prevents us from moving this to dpu and start using
catalog for this?
>>
>> /* struct dpu_mdp_cfg : MDP TOP-BLK instance info
>> * @id: index identifying this block
>> * @base: register base offset to mdss
>> * @features bit mask identifying sub-blocks/features
>> * @highest_bank_bit: UBWC parameter
>> * @ubwc_static: ubwc static configuration
>> * @ubwc_swizzle: ubwc default swizzle setting
>> * @clk_ctrls clock control register definition
>> */
>> struct dpu_mdp_cfg {
>> DPU_HW_BLK_INFO;
>> u32 highest_bank_bit;
>> u32 ubwc_swizzle;
>> struct dpu_clk_ctrl_reg clk_ctrls[DPU_CLK_CTRL_MAX];
>> };
>>
>> We already do seem to have a couple of parameters. have to add the others.
>>
>> That way the number of functions wont keep growing.
>>
>>>>
>>>>>>
>>>>>>> static int msm_mdss_enable(struct msm_mdss *msm_mdss)
>>>>>>> {
>>>>>>> int ret;
>>>>>>> + u32 hw_rev;
>>>>>>>
>>>>>>> ret = clk_bulk_prepare_enable(msm_mdss->num_clocks, msm_mdss->clocks);
>>>>>>> if (ret) {
>>>>>>> @@ -149,26 +204,34 @@ static int msm_mdss_enable(struct msm_mdss *msm_mdss)
>>>>>>> if (msm_mdss->is_mdp5)
>>>>>>> return 0;
>>>>>>>
>>>>>>> + hw_rev = readl_relaxed(msm_mdss->mmio + HW_REV);
>>>>>>> + dev_info(msm_mdss->dev, "HW_REV: 0x%x\n", hw_rev);
>>>>>>> + dev_info(msm_mdss->dev, "UBWC_DEC_HW_VERSION: 0x%x\n",
>>>>>>> + readl_relaxed(msm_mdss->mmio + UBWC_DEC_HW_VERSION));
>>>>>>
>>>>>> we are already printing the HW version here
>>>>>>
>>>>>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c#L1096
>>>>>>
>>>>>> Do you want to remove that print then? May be. Let me take a look.
>>>>>
>>>>> [skipped]
>>>>>
>>>
>>>
>>>
>
>
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Freedreno] [PATCH] drm/msm: less magic numbers in msm_mdss_enable
@ 2022-06-08 19:29 ` Abhinav Kumar
0 siblings, 0 replies; 32+ messages in thread
From: Abhinav Kumar @ 2022-06-08 19:29 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Sean Paul, David Airlie, linux-arm-msm, dri-devel,
Bjorn Andersson, Stephen Boyd, freedreno
On 6/2/2022 1:13 PM, Dmitry Baryshkov wrote:
> On Thu, 2 Jun 2022 at 21:18, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 6/1/2022 1:04 PM, Dmitry Baryshkov wrote:
>>> On Wed, 1 Jun 2022 at 20:38, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 6/1/2022 2:46 AM, Dmitry Baryshkov wrote:
>>>>> On Wed, 1 Jun 2022 at 01:01, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>>>>> On 5/31/2022 5:18 AM, Dmitry Baryshkov wrote:
>>>>>>> Replace magic register writes in msm_mdss_enable() with version that
>>>>>>> contains less magic and more variable names that can be traced back to
>>>>>>> the dpu_hw_catalog or the downstream dtsi files.
>>>>>>>
>>>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>>> ---
>>>>>>> drivers/gpu/drm/msm/msm_mdss.c | 79 ++++++++++++++++++++++++++++++----
>>>>>>> 1 file changed, 71 insertions(+), 8 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>> index 0454a571adf7..2a48263cd1b5 100644
>>>>>>> --- a/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>> +++ b/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>> @@ -21,6 +21,7 @@
>>>>>>> #define HW_REV 0x0
>>>>>>> #define HW_INTR_STATUS 0x0010
>>>>>>>
>>>>>>> +#define UBWC_DEC_HW_VERSION 0x58
>>>>>>> #define UBWC_STATIC 0x144
>>>>>>> #define UBWC_CTRL_2 0x150
>>>>>>> #define UBWC_PREDICTION_MODE 0x154
>>>>>>> @@ -132,9 +133,63 @@ static int _msm_mdss_irq_domain_add(struct msm_mdss *msm_mdss)
>>>>>>> return 0;
>>>>>>> }
>>>>>>>
>>>>>>> +#define UBWC_1_0 0x10000000
>>>>>>> +#define UBWC_2_0 0x20000000
>>>>>>> +#define UBWC_3_0 0x30000000
>>>>>>> +#define UBWC_4_0 0x40000000
>>>>>>> +
>>>>>>> +static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss,
>>>>>>> + u32 ubwc_static)
>>>>>>> +{
>>>>>>> + writel_relaxed(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)
>>>>>>> +{
>>>>>>> + u32 value = (ubwc_swizzle & 0x1) |
>>>>>>> + (highest_bank_bit & 0x3) << 4 |
>>>>>>> + (macrotile_mode & 0x1) << 12;
>>>>>>> +
>>>>>>> + if (ubwc_version == UBWC_3_0)
>>>>>>> + value |= BIT(10);
>>>>>>> +
>>>>>>> + if (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)
>>>>>>> +{
>>>>>>> + u32 value = (ubwc_swizzle & 0x7) |
>>>>>>> + (ubwc_static & 0x1) << 3 |
>>>>>>> + (highest_bank_bit & 0x7) << 4 |
>>>>>>> + (macrotile_mode & 0x1) << 12;
>>>>>>> +
>>>>>>> + writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
>>>>>>> +
>>>>>>> + if (ubwc_version == UBWC_3_0) {
>>>>>>> + writel_relaxed(1, msm_mdss->mmio + UBWC_CTRL_2);
>>>>>>> + writel_relaxed(0, msm_mdss->mmio + UBWC_PREDICTION_MODE);
>>>>>>> + } else {
>>>>>>> + writel_relaxed(2, msm_mdss->mmio + UBWC_CTRL_2);
>>>>>>> + writel_relaxed(1, msm_mdss->mmio + UBWC_PREDICTION_MODE);
>>>>>>> + }
>>>>>>> +}
>>>>>>> +
>>>>>>
>>>>>> Is it possible to unify the above functions by having the internal
>>>>>> ubwc_version checks?
>>>>>
>>>>> Note, it's not the ubwc_version, it is the ubwc_dec_hw_version. And
>>>>> also different functions take different sets of arguments.
>>>>>
>>>>>> It seems like msm_mdss_setup_ubwc_dec_xxx can keep growing.
>>>>>>
>>>>>> I have not looked into each bit programming but from the top level so
>>>>>> feel free to correct if wrong but it seems both do write UBWC_STATIC
>>>>>> (different values based on different UBWC versions) and write some extra
>>>>>> registers based on version
>>>>>
>>>>> This is what both the current code and the downstream do. See
>>>>> https://github.com/MiCode/Xiaomi_Kernel_OpenSource/blob/zeus-s-oss/techpack/display-drivers/msm/sde/sde_hw_top.c#L312
>>>>>
>>>>
>>>> Thanks for pointing to the downstream method for this,
>>>>
>>>> This is exactly what i was also suggesting to do when I mentioned
>>>> unifying the above functions.
>>>>
>>>> So instead of having a separate function for each version why not handle
>>>> all the versions in the same function like what the link you have shown
>>>> does.
>>>
>>> I wouldn't like that. The downstream uses hw_catalog to pass all
>>> possible parameters. We do not, so we'd have a whole set of artificial
>>> values.
>>>
>>
>> Now that you brought that up, why cannot even upstream dpu start using
>> catalog for ubwc settings?
>
> Because msm_mdss lives out of disp/dpu1. And using the disp/dpu1 for
> it would be an inversion of dependencies.
> I like the fact that msm_mdss is independent of mdp/dpu drivers and I
> do not want to add such dependency.
>
Ok, so I think this function itself is placed incorrectly. It should not
be in msm_mdss.c and should in the DPU folder.
This check tells me that this will not be executed for mdp5 devices.
/*
* 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)
return 0;
In that case, what prevents us from moving this to dpu and start using
catalog for this?
>>
>> /* struct dpu_mdp_cfg : MDP TOP-BLK instance info
>> * @id: index identifying this block
>> * @base: register base offset to mdss
>> * @features bit mask identifying sub-blocks/features
>> * @highest_bank_bit: UBWC parameter
>> * @ubwc_static: ubwc static configuration
>> * @ubwc_swizzle: ubwc default swizzle setting
>> * @clk_ctrls clock control register definition
>> */
>> struct dpu_mdp_cfg {
>> DPU_HW_BLK_INFO;
>> u32 highest_bank_bit;
>> u32 ubwc_swizzle;
>> struct dpu_clk_ctrl_reg clk_ctrls[DPU_CLK_CTRL_MAX];
>> };
>>
>> We already do seem to have a couple of parameters. have to add the others.
>>
>> That way the number of functions wont keep growing.
>>
>>>>
>>>>>>
>>>>>>> static int msm_mdss_enable(struct msm_mdss *msm_mdss)
>>>>>>> {
>>>>>>> int ret;
>>>>>>> + u32 hw_rev;
>>>>>>>
>>>>>>> ret = clk_bulk_prepare_enable(msm_mdss->num_clocks, msm_mdss->clocks);
>>>>>>> if (ret) {
>>>>>>> @@ -149,26 +204,34 @@ static int msm_mdss_enable(struct msm_mdss *msm_mdss)
>>>>>>> if (msm_mdss->is_mdp5)
>>>>>>> return 0;
>>>>>>>
>>>>>>> + hw_rev = readl_relaxed(msm_mdss->mmio + HW_REV);
>>>>>>> + dev_info(msm_mdss->dev, "HW_REV: 0x%x\n", hw_rev);
>>>>>>> + dev_info(msm_mdss->dev, "UBWC_DEC_HW_VERSION: 0x%x\n",
>>>>>>> + readl_relaxed(msm_mdss->mmio + UBWC_DEC_HW_VERSION));
>>>>>>
>>>>>> we are already printing the HW version here
>>>>>>
>>>>>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c#L1096
>>>>>>
>>>>>> Do you want to remove that print then? May be. Let me take a look.
>>>>>
>>>>> [skipped]
>>>>>
>>>
>>>
>>>
>
>
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Freedreno] [PATCH] drm/msm: less magic numbers in msm_mdss_enable
2022-06-08 19:29 ` Abhinav Kumar
@ 2022-06-08 22:30 ` Dmitry Baryshkov
-1 siblings, 0 replies; 32+ messages in thread
From: Dmitry Baryshkov @ 2022-06-08 22:30 UTC (permalink / raw)
To: Abhinav Kumar
Cc: Sean Paul, David Airlie, linux-arm-msm, dri-devel,
Bjorn Andersson, Stephen Boyd, freedreno
On Wed, 8 Jun 2022 at 22:29, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 6/2/2022 1:13 PM, Dmitry Baryshkov wrote:
> > On Thu, 2 Jun 2022 at 21:18, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>
> >>
> >>
> >> On 6/1/2022 1:04 PM, Dmitry Baryshkov wrote:
> >>> On Wed, 1 Jun 2022 at 20:38, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 6/1/2022 2:46 AM, Dmitry Baryshkov wrote:
> >>>>> On Wed, 1 Jun 2022 at 01:01, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>>>>> On 5/31/2022 5:18 AM, Dmitry Baryshkov wrote:
> >>>>>>> Replace magic register writes in msm_mdss_enable() with version that
> >>>>>>> contains less magic and more variable names that can be traced back to
> >>>>>>> the dpu_hw_catalog or the downstream dtsi files.
> >>>>>>>
> >>>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >>>>>>> ---
> >>>>>>> drivers/gpu/drm/msm/msm_mdss.c | 79 ++++++++++++++++++++++++++++++----
> >>>>>>> 1 file changed, 71 insertions(+), 8 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
> >>>>>>> index 0454a571adf7..2a48263cd1b5 100644
> >>>>>>> --- a/drivers/gpu/drm/msm/msm_mdss.c
> >>>>>>> +++ b/drivers/gpu/drm/msm/msm_mdss.c
> >>>>>>> @@ -21,6 +21,7 @@
> >>>>>>> #define HW_REV 0x0
> >>>>>>> #define HW_INTR_STATUS 0x0010
> >>>>>>>
> >>>>>>> +#define UBWC_DEC_HW_VERSION 0x58
> >>>>>>> #define UBWC_STATIC 0x144
> >>>>>>> #define UBWC_CTRL_2 0x150
> >>>>>>> #define UBWC_PREDICTION_MODE 0x154
> >>>>>>> @@ -132,9 +133,63 @@ static int _msm_mdss_irq_domain_add(struct msm_mdss *msm_mdss)
> >>>>>>> return 0;
> >>>>>>> }
> >>>>>>>
> >>>>>>> +#define UBWC_1_0 0x10000000
> >>>>>>> +#define UBWC_2_0 0x20000000
> >>>>>>> +#define UBWC_3_0 0x30000000
> >>>>>>> +#define UBWC_4_0 0x40000000
> >>>>>>> +
> >>>>>>> +static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss,
> >>>>>>> + u32 ubwc_static)
> >>>>>>> +{
> >>>>>>> + writel_relaxed(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)
> >>>>>>> +{
> >>>>>>> + u32 value = (ubwc_swizzle & 0x1) |
> >>>>>>> + (highest_bank_bit & 0x3) << 4 |
> >>>>>>> + (macrotile_mode & 0x1) << 12;
> >>>>>>> +
> >>>>>>> + if (ubwc_version == UBWC_3_0)
> >>>>>>> + value |= BIT(10);
> >>>>>>> +
> >>>>>>> + if (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)
> >>>>>>> +{
> >>>>>>> + u32 value = (ubwc_swizzle & 0x7) |
> >>>>>>> + (ubwc_static & 0x1) << 3 |
> >>>>>>> + (highest_bank_bit & 0x7) << 4 |
> >>>>>>> + (macrotile_mode & 0x1) << 12;
> >>>>>>> +
> >>>>>>> + writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
> >>>>>>> +
> >>>>>>> + if (ubwc_version == UBWC_3_0) {
> >>>>>>> + writel_relaxed(1, msm_mdss->mmio + UBWC_CTRL_2);
> >>>>>>> + writel_relaxed(0, msm_mdss->mmio + UBWC_PREDICTION_MODE);
> >>>>>>> + } else {
> >>>>>>> + writel_relaxed(2, msm_mdss->mmio + UBWC_CTRL_2);
> >>>>>>> + writel_relaxed(1, msm_mdss->mmio + UBWC_PREDICTION_MODE);
> >>>>>>> + }
> >>>>>>> +}
> >>>>>>> +
> >>>>>>
> >>>>>> Is it possible to unify the above functions by having the internal
> >>>>>> ubwc_version checks?
> >>>>>
> >>>>> Note, it's not the ubwc_version, it is the ubwc_dec_hw_version. And
> >>>>> also different functions take different sets of arguments.
> >>>>>
> >>>>>> It seems like msm_mdss_setup_ubwc_dec_xxx can keep growing.
> >>>>>>
> >>>>>> I have not looked into each bit programming but from the top level so
> >>>>>> feel free to correct if wrong but it seems both do write UBWC_STATIC
> >>>>>> (different values based on different UBWC versions) and write some extra
> >>>>>> registers based on version
> >>>>>
> >>>>> This is what both the current code and the downstream do. See
> >>>>> https://github.com/MiCode/Xiaomi_Kernel_OpenSource/blob/zeus-s-oss/techpack/display-drivers/msm/sde/sde_hw_top.c#L312
> >>>>>
> >>>>
> >>>> Thanks for pointing to the downstream method for this,
> >>>>
> >>>> This is exactly what i was also suggesting to do when I mentioned
> >>>> unifying the above functions.
> >>>>
> >>>> So instead of having a separate function for each version why not handle
> >>>> all the versions in the same function like what the link you have shown
> >>>> does.
> >>>
> >>> I wouldn't like that. The downstream uses hw_catalog to pass all
> >>> possible parameters. We do not, so we'd have a whole set of artificial
> >>> values.
> >>>
> >>
> >> Now that you brought that up, why cannot even upstream dpu start using
> >> catalog for ubwc settings?
> >
> > Because msm_mdss lives out of disp/dpu1. And using the disp/dpu1 for
> > it would be an inversion of dependencies.
> > I like the fact that msm_mdss is independent of mdp/dpu drivers and I
> > do not want to add such dependency.
> >
>
> Ok, so I think this function itself is placed incorrectly. It should not
> be in msm_mdss.c and should in the DPU folder.
>
> This check tells me that this will not be executed for mdp5 devices.
>
> /*
> * 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)
> return 0;
This condition should be changed to check for the MDP_CLK being
available in the clocks array rather than checking for is_mdp5. I'd
like to phase is_mdp5 away at some point.
> In that case, what prevents us from moving this to dpu and start using
> catalog for this?
Because there is nothing tying mdss and dpu drivers. For example, is
the msm8998 (3.0.0) the DPU or MDP5 device? MSM8996?
Neither struct msm_mdss nor the MDSS device itself are accessible
through the msm_drv (or dpu_kms).
I think trying to invent such a link would make the code worse.
> >> /* struct dpu_mdp_cfg : MDP TOP-BLK instance info
> >> * @id: index identifying this block
> >> * @base: register base offset to mdss
> >> * @features bit mask identifying sub-blocks/features
> >> * @highest_bank_bit: UBWC parameter
> >> * @ubwc_static: ubwc static configuration
> >> * @ubwc_swizzle: ubwc default swizzle setting
> >> * @clk_ctrls clock control register definition
> >> */
> >> struct dpu_mdp_cfg {
> >> DPU_HW_BLK_INFO;
> >> u32 highest_bank_bit;
> >> u32 ubwc_swizzle;
> >> struct dpu_clk_ctrl_reg clk_ctrls[DPU_CLK_CTRL_MAX];
> >> };
> >>
> >> We already do seem to have a couple of parameters. have to add the others.
> >>
> >> That way the number of functions wont keep growing.
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Freedreno] [PATCH] drm/msm: less magic numbers in msm_mdss_enable
@ 2022-06-08 22:30 ` Dmitry Baryshkov
0 siblings, 0 replies; 32+ messages in thread
From: Dmitry Baryshkov @ 2022-06-08 22:30 UTC (permalink / raw)
To: Abhinav Kumar
Cc: freedreno, David Airlie, linux-arm-msm, dri-devel, Stephen Boyd,
Bjorn Andersson, Sean Paul
On Wed, 8 Jun 2022 at 22:29, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 6/2/2022 1:13 PM, Dmitry Baryshkov wrote:
> > On Thu, 2 Jun 2022 at 21:18, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>
> >>
> >>
> >> On 6/1/2022 1:04 PM, Dmitry Baryshkov wrote:
> >>> On Wed, 1 Jun 2022 at 20:38, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 6/1/2022 2:46 AM, Dmitry Baryshkov wrote:
> >>>>> On Wed, 1 Jun 2022 at 01:01, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>>>>> On 5/31/2022 5:18 AM, Dmitry Baryshkov wrote:
> >>>>>>> Replace magic register writes in msm_mdss_enable() with version that
> >>>>>>> contains less magic and more variable names that can be traced back to
> >>>>>>> the dpu_hw_catalog or the downstream dtsi files.
> >>>>>>>
> >>>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >>>>>>> ---
> >>>>>>> drivers/gpu/drm/msm/msm_mdss.c | 79 ++++++++++++++++++++++++++++++----
> >>>>>>> 1 file changed, 71 insertions(+), 8 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
> >>>>>>> index 0454a571adf7..2a48263cd1b5 100644
> >>>>>>> --- a/drivers/gpu/drm/msm/msm_mdss.c
> >>>>>>> +++ b/drivers/gpu/drm/msm/msm_mdss.c
> >>>>>>> @@ -21,6 +21,7 @@
> >>>>>>> #define HW_REV 0x0
> >>>>>>> #define HW_INTR_STATUS 0x0010
> >>>>>>>
> >>>>>>> +#define UBWC_DEC_HW_VERSION 0x58
> >>>>>>> #define UBWC_STATIC 0x144
> >>>>>>> #define UBWC_CTRL_2 0x150
> >>>>>>> #define UBWC_PREDICTION_MODE 0x154
> >>>>>>> @@ -132,9 +133,63 @@ static int _msm_mdss_irq_domain_add(struct msm_mdss *msm_mdss)
> >>>>>>> return 0;
> >>>>>>> }
> >>>>>>>
> >>>>>>> +#define UBWC_1_0 0x10000000
> >>>>>>> +#define UBWC_2_0 0x20000000
> >>>>>>> +#define UBWC_3_0 0x30000000
> >>>>>>> +#define UBWC_4_0 0x40000000
> >>>>>>> +
> >>>>>>> +static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss,
> >>>>>>> + u32 ubwc_static)
> >>>>>>> +{
> >>>>>>> + writel_relaxed(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)
> >>>>>>> +{
> >>>>>>> + u32 value = (ubwc_swizzle & 0x1) |
> >>>>>>> + (highest_bank_bit & 0x3) << 4 |
> >>>>>>> + (macrotile_mode & 0x1) << 12;
> >>>>>>> +
> >>>>>>> + if (ubwc_version == UBWC_3_0)
> >>>>>>> + value |= BIT(10);
> >>>>>>> +
> >>>>>>> + if (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)
> >>>>>>> +{
> >>>>>>> + u32 value = (ubwc_swizzle & 0x7) |
> >>>>>>> + (ubwc_static & 0x1) << 3 |
> >>>>>>> + (highest_bank_bit & 0x7) << 4 |
> >>>>>>> + (macrotile_mode & 0x1) << 12;
> >>>>>>> +
> >>>>>>> + writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
> >>>>>>> +
> >>>>>>> + if (ubwc_version == UBWC_3_0) {
> >>>>>>> + writel_relaxed(1, msm_mdss->mmio + UBWC_CTRL_2);
> >>>>>>> + writel_relaxed(0, msm_mdss->mmio + UBWC_PREDICTION_MODE);
> >>>>>>> + } else {
> >>>>>>> + writel_relaxed(2, msm_mdss->mmio + UBWC_CTRL_2);
> >>>>>>> + writel_relaxed(1, msm_mdss->mmio + UBWC_PREDICTION_MODE);
> >>>>>>> + }
> >>>>>>> +}
> >>>>>>> +
> >>>>>>
> >>>>>> Is it possible to unify the above functions by having the internal
> >>>>>> ubwc_version checks?
> >>>>>
> >>>>> Note, it's not the ubwc_version, it is the ubwc_dec_hw_version. And
> >>>>> also different functions take different sets of arguments.
> >>>>>
> >>>>>> It seems like msm_mdss_setup_ubwc_dec_xxx can keep growing.
> >>>>>>
> >>>>>> I have not looked into each bit programming but from the top level so
> >>>>>> feel free to correct if wrong but it seems both do write UBWC_STATIC
> >>>>>> (different values based on different UBWC versions) and write some extra
> >>>>>> registers based on version
> >>>>>
> >>>>> This is what both the current code and the downstream do. See
> >>>>> https://github.com/MiCode/Xiaomi_Kernel_OpenSource/blob/zeus-s-oss/techpack/display-drivers/msm/sde/sde_hw_top.c#L312
> >>>>>
> >>>>
> >>>> Thanks for pointing to the downstream method for this,
> >>>>
> >>>> This is exactly what i was also suggesting to do when I mentioned
> >>>> unifying the above functions.
> >>>>
> >>>> So instead of having a separate function for each version why not handle
> >>>> all the versions in the same function like what the link you have shown
> >>>> does.
> >>>
> >>> I wouldn't like that. The downstream uses hw_catalog to pass all
> >>> possible parameters. We do not, so we'd have a whole set of artificial
> >>> values.
> >>>
> >>
> >> Now that you brought that up, why cannot even upstream dpu start using
> >> catalog for ubwc settings?
> >
> > Because msm_mdss lives out of disp/dpu1. And using the disp/dpu1 for
> > it would be an inversion of dependencies.
> > I like the fact that msm_mdss is independent of mdp/dpu drivers and I
> > do not want to add such dependency.
> >
>
> Ok, so I think this function itself is placed incorrectly. It should not
> be in msm_mdss.c and should in the DPU folder.
>
> This check tells me that this will not be executed for mdp5 devices.
>
> /*
> * 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)
> return 0;
This condition should be changed to check for the MDP_CLK being
available in the clocks array rather than checking for is_mdp5. I'd
like to phase is_mdp5 away at some point.
> In that case, what prevents us from moving this to dpu and start using
> catalog for this?
Because there is nothing tying mdss and dpu drivers. For example, is
the msm8998 (3.0.0) the DPU or MDP5 device? MSM8996?
Neither struct msm_mdss nor the MDSS device itself are accessible
through the msm_drv (or dpu_kms).
I think trying to invent such a link would make the code worse.
> >> /* struct dpu_mdp_cfg : MDP TOP-BLK instance info
> >> * @id: index identifying this block
> >> * @base: register base offset to mdss
> >> * @features bit mask identifying sub-blocks/features
> >> * @highest_bank_bit: UBWC parameter
> >> * @ubwc_static: ubwc static configuration
> >> * @ubwc_swizzle: ubwc default swizzle setting
> >> * @clk_ctrls clock control register definition
> >> */
> >> struct dpu_mdp_cfg {
> >> DPU_HW_BLK_INFO;
> >> u32 highest_bank_bit;
> >> u32 ubwc_swizzle;
> >> struct dpu_clk_ctrl_reg clk_ctrls[DPU_CLK_CTRL_MAX];
> >> };
> >>
> >> We already do seem to have a couple of parameters. have to add the others.
> >>
> >> That way the number of functions wont keep growing.
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Freedreno] [PATCH] drm/msm: less magic numbers in msm_mdss_enable
2022-06-08 22:30 ` Dmitry Baryshkov
@ 2022-06-08 22:35 ` Abhinav Kumar
-1 siblings, 0 replies; 32+ messages in thread
From: Abhinav Kumar @ 2022-06-08 22:35 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: freedreno, David Airlie, linux-arm-msm, dri-devel, Stephen Boyd,
Bjorn Andersson, Sean Paul
On 6/8/2022 3:30 PM, Dmitry Baryshkov wrote:
> On Wed, 8 Jun 2022 at 22:29, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 6/2/2022 1:13 PM, Dmitry Baryshkov wrote:
>>> On Thu, 2 Jun 2022 at 21:18, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 6/1/2022 1:04 PM, Dmitry Baryshkov wrote:
>>>>> On Wed, 1 Jun 2022 at 20:38, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 6/1/2022 2:46 AM, Dmitry Baryshkov wrote:
>>>>>>> On Wed, 1 Jun 2022 at 01:01, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>>>>>>> On 5/31/2022 5:18 AM, Dmitry Baryshkov wrote:
>>>>>>>>> Replace magic register writes in msm_mdss_enable() with version that
>>>>>>>>> contains less magic and more variable names that can be traced back to
>>>>>>>>> the dpu_hw_catalog or the downstream dtsi files.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>>>>> ---
>>>>>>>>> drivers/gpu/drm/msm/msm_mdss.c | 79 ++++++++++++++++++++++++++++++----
>>>>>>>>> 1 file changed, 71 insertions(+), 8 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>>>> index 0454a571adf7..2a48263cd1b5 100644
>>>>>>>>> --- a/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>>>> +++ b/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>>>> @@ -21,6 +21,7 @@
>>>>>>>>> #define HW_REV 0x0
>>>>>>>>> #define HW_INTR_STATUS 0x0010
>>>>>>>>>
>>>>>>>>> +#define UBWC_DEC_HW_VERSION 0x58
>>>>>>>>> #define UBWC_STATIC 0x144
>>>>>>>>> #define UBWC_CTRL_2 0x150
>>>>>>>>> #define UBWC_PREDICTION_MODE 0x154
>>>>>>>>> @@ -132,9 +133,63 @@ static int _msm_mdss_irq_domain_add(struct msm_mdss *msm_mdss)
>>>>>>>>> return 0;
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> +#define UBWC_1_0 0x10000000
>>>>>>>>> +#define UBWC_2_0 0x20000000
>>>>>>>>> +#define UBWC_3_0 0x30000000
>>>>>>>>> +#define UBWC_4_0 0x40000000
>>>>>>>>> +
>>>>>>>>> +static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss,
>>>>>>>>> + u32 ubwc_static)
>>>>>>>>> +{
>>>>>>>>> + writel_relaxed(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)
>>>>>>>>> +{
>>>>>>>>> + u32 value = (ubwc_swizzle & 0x1) |
>>>>>>>>> + (highest_bank_bit & 0x3) << 4 |
>>>>>>>>> + (macrotile_mode & 0x1) << 12;
>>>>>>>>> +
>>>>>>>>> + if (ubwc_version == UBWC_3_0)
>>>>>>>>> + value |= BIT(10);
>>>>>>>>> +
>>>>>>>>> + if (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)
>>>>>>>>> +{
>>>>>>>>> + u32 value = (ubwc_swizzle & 0x7) |
>>>>>>>>> + (ubwc_static & 0x1) << 3 |
>>>>>>>>> + (highest_bank_bit & 0x7) << 4 |
>>>>>>>>> + (macrotile_mode & 0x1) << 12;
>>>>>>>>> +
>>>>>>>>> + writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
>>>>>>>>> +
>>>>>>>>> + if (ubwc_version == UBWC_3_0) {
>>>>>>>>> + writel_relaxed(1, msm_mdss->mmio + UBWC_CTRL_2);
>>>>>>>>> + writel_relaxed(0, msm_mdss->mmio + UBWC_PREDICTION_MODE);
>>>>>>>>> + } else {
>>>>>>>>> + writel_relaxed(2, msm_mdss->mmio + UBWC_CTRL_2);
>>>>>>>>> + writel_relaxed(1, msm_mdss->mmio + UBWC_PREDICTION_MODE);
>>>>>>>>> + }
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>
>>>>>>>> Is it possible to unify the above functions by having the internal
>>>>>>>> ubwc_version checks?
>>>>>>>
>>>>>>> Note, it's not the ubwc_version, it is the ubwc_dec_hw_version. And
>>>>>>> also different functions take different sets of arguments.
>>>>>>>
>>>>>>>> It seems like msm_mdss_setup_ubwc_dec_xxx can keep growing.
>>>>>>>>
>>>>>>>> I have not looked into each bit programming but from the top level so
>>>>>>>> feel free to correct if wrong but it seems both do write UBWC_STATIC
>>>>>>>> (different values based on different UBWC versions) and write some extra
>>>>>>>> registers based on version
>>>>>>>
>>>>>>> This is what both the current code and the downstream do. See
>>>>>>> https://github.com/MiCode/Xiaomi_Kernel_OpenSource/blob/zeus-s-oss/techpack/display-drivers/msm/sde/sde_hw_top.c#L312
>>>>>>>
>>>>>>
>>>>>> Thanks for pointing to the downstream method for this,
>>>>>>
>>>>>> This is exactly what i was also suggesting to do when I mentioned
>>>>>> unifying the above functions.
>>>>>>
>>>>>> So instead of having a separate function for each version why not handle
>>>>>> all the versions in the same function like what the link you have shown
>>>>>> does.
>>>>>
>>>>> I wouldn't like that. The downstream uses hw_catalog to pass all
>>>>> possible parameters. We do not, so we'd have a whole set of artificial
>>>>> values.
>>>>>
>>>>
>>>> Now that you brought that up, why cannot even upstream dpu start using
>>>> catalog for ubwc settings?
>>>
>>> Because msm_mdss lives out of disp/dpu1. And using the disp/dpu1 for
>>> it would be an inversion of dependencies.
>>> I like the fact that msm_mdss is independent of mdp/dpu drivers and I
>>> do not want to add such dependency.
>>>
>>
>> Ok, so I think this function itself is placed incorrectly. It should not
>> be in msm_mdss.c and should in the DPU folder.
>>
>> This check tells me that this will not be executed for mdp5 devices.
>>
>> /*
>> * 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)
>> return 0;
>
> This condition should be changed to check for the MDP_CLK being
> available in the clocks array rather than checking for is_mdp5. I'd
> like to phase is_mdp5 away at some point.
>
>> In that case, what prevents us from moving this to dpu and start using
>> catalog for this?
>
> Because there is nothing tying mdss and dpu drivers. For example, is
> the msm8998 (3.0.0) the DPU or MDP5 device? MSM8996?
> Neither struct msm_mdss nor the MDSS device itself are accessible
> through the msm_drv (or dpu_kms).
> I think trying to invent such a link would make the code worse.
>
Right, what I am trying to mention with that check is that means that
code does not run today for mdp5 and it still works fine.
So why not just move it to DPU first to carry less burden of these extra
register settings which are unused today for mdp5 anyway.
>>>> /* struct dpu_mdp_cfg : MDP TOP-BLK instance info
>>>> * @id: index identifying this block
>>>> * @base: register base offset to mdss
>>>> * @features bit mask identifying sub-blocks/features
>>>> * @highest_bank_bit: UBWC parameter
>>>> * @ubwc_static: ubwc static configuration
>>>> * @ubwc_swizzle: ubwc default swizzle setting
>>>> * @clk_ctrls clock control register definition
>>>> */
>>>> struct dpu_mdp_cfg {
>>>> DPU_HW_BLK_INFO;
>>>> u32 highest_bank_bit;
>>>> u32 ubwc_swizzle;
>>>> struct dpu_clk_ctrl_reg clk_ctrls[DPU_CLK_CTRL_MAX];
>>>> };
>>>>
>>>> We already do seem to have a couple of parameters. have to add the others.
>>>>
>>>> That way the number of functions wont keep growing.
>
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Freedreno] [PATCH] drm/msm: less magic numbers in msm_mdss_enable
@ 2022-06-08 22:35 ` Abhinav Kumar
0 siblings, 0 replies; 32+ messages in thread
From: Abhinav Kumar @ 2022-06-08 22:35 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Sean Paul, David Airlie, linux-arm-msm, dri-devel,
Bjorn Andersson, Stephen Boyd, freedreno
On 6/8/2022 3:30 PM, Dmitry Baryshkov wrote:
> On Wed, 8 Jun 2022 at 22:29, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 6/2/2022 1:13 PM, Dmitry Baryshkov wrote:
>>> On Thu, 2 Jun 2022 at 21:18, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 6/1/2022 1:04 PM, Dmitry Baryshkov wrote:
>>>>> On Wed, 1 Jun 2022 at 20:38, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 6/1/2022 2:46 AM, Dmitry Baryshkov wrote:
>>>>>>> On Wed, 1 Jun 2022 at 01:01, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>>>>>>> On 5/31/2022 5:18 AM, Dmitry Baryshkov wrote:
>>>>>>>>> Replace magic register writes in msm_mdss_enable() with version that
>>>>>>>>> contains less magic and more variable names that can be traced back to
>>>>>>>>> the dpu_hw_catalog or the downstream dtsi files.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>>>>> ---
>>>>>>>>> drivers/gpu/drm/msm/msm_mdss.c | 79 ++++++++++++++++++++++++++++++----
>>>>>>>>> 1 file changed, 71 insertions(+), 8 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>>>> index 0454a571adf7..2a48263cd1b5 100644
>>>>>>>>> --- a/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>>>> +++ b/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>>>> @@ -21,6 +21,7 @@
>>>>>>>>> #define HW_REV 0x0
>>>>>>>>> #define HW_INTR_STATUS 0x0010
>>>>>>>>>
>>>>>>>>> +#define UBWC_DEC_HW_VERSION 0x58
>>>>>>>>> #define UBWC_STATIC 0x144
>>>>>>>>> #define UBWC_CTRL_2 0x150
>>>>>>>>> #define UBWC_PREDICTION_MODE 0x154
>>>>>>>>> @@ -132,9 +133,63 @@ static int _msm_mdss_irq_domain_add(struct msm_mdss *msm_mdss)
>>>>>>>>> return 0;
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> +#define UBWC_1_0 0x10000000
>>>>>>>>> +#define UBWC_2_0 0x20000000
>>>>>>>>> +#define UBWC_3_0 0x30000000
>>>>>>>>> +#define UBWC_4_0 0x40000000
>>>>>>>>> +
>>>>>>>>> +static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss,
>>>>>>>>> + u32 ubwc_static)
>>>>>>>>> +{
>>>>>>>>> + writel_relaxed(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)
>>>>>>>>> +{
>>>>>>>>> + u32 value = (ubwc_swizzle & 0x1) |
>>>>>>>>> + (highest_bank_bit & 0x3) << 4 |
>>>>>>>>> + (macrotile_mode & 0x1) << 12;
>>>>>>>>> +
>>>>>>>>> + if (ubwc_version == UBWC_3_0)
>>>>>>>>> + value |= BIT(10);
>>>>>>>>> +
>>>>>>>>> + if (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)
>>>>>>>>> +{
>>>>>>>>> + u32 value = (ubwc_swizzle & 0x7) |
>>>>>>>>> + (ubwc_static & 0x1) << 3 |
>>>>>>>>> + (highest_bank_bit & 0x7) << 4 |
>>>>>>>>> + (macrotile_mode & 0x1) << 12;
>>>>>>>>> +
>>>>>>>>> + writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
>>>>>>>>> +
>>>>>>>>> + if (ubwc_version == UBWC_3_0) {
>>>>>>>>> + writel_relaxed(1, msm_mdss->mmio + UBWC_CTRL_2);
>>>>>>>>> + writel_relaxed(0, msm_mdss->mmio + UBWC_PREDICTION_MODE);
>>>>>>>>> + } else {
>>>>>>>>> + writel_relaxed(2, msm_mdss->mmio + UBWC_CTRL_2);
>>>>>>>>> + writel_relaxed(1, msm_mdss->mmio + UBWC_PREDICTION_MODE);
>>>>>>>>> + }
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>
>>>>>>>> Is it possible to unify the above functions by having the internal
>>>>>>>> ubwc_version checks?
>>>>>>>
>>>>>>> Note, it's not the ubwc_version, it is the ubwc_dec_hw_version. And
>>>>>>> also different functions take different sets of arguments.
>>>>>>>
>>>>>>>> It seems like msm_mdss_setup_ubwc_dec_xxx can keep growing.
>>>>>>>>
>>>>>>>> I have not looked into each bit programming but from the top level so
>>>>>>>> feel free to correct if wrong but it seems both do write UBWC_STATIC
>>>>>>>> (different values based on different UBWC versions) and write some extra
>>>>>>>> registers based on version
>>>>>>>
>>>>>>> This is what both the current code and the downstream do. See
>>>>>>> https://github.com/MiCode/Xiaomi_Kernel_OpenSource/blob/zeus-s-oss/techpack/display-drivers/msm/sde/sde_hw_top.c#L312
>>>>>>>
>>>>>>
>>>>>> Thanks for pointing to the downstream method for this,
>>>>>>
>>>>>> This is exactly what i was also suggesting to do when I mentioned
>>>>>> unifying the above functions.
>>>>>>
>>>>>> So instead of having a separate function for each version why not handle
>>>>>> all the versions in the same function like what the link you have shown
>>>>>> does.
>>>>>
>>>>> I wouldn't like that. The downstream uses hw_catalog to pass all
>>>>> possible parameters. We do not, so we'd have a whole set of artificial
>>>>> values.
>>>>>
>>>>
>>>> Now that you brought that up, why cannot even upstream dpu start using
>>>> catalog for ubwc settings?
>>>
>>> Because msm_mdss lives out of disp/dpu1. And using the disp/dpu1 for
>>> it would be an inversion of dependencies.
>>> I like the fact that msm_mdss is independent of mdp/dpu drivers and I
>>> do not want to add such dependency.
>>>
>>
>> Ok, so I think this function itself is placed incorrectly. It should not
>> be in msm_mdss.c and should in the DPU folder.
>>
>> This check tells me that this will not be executed for mdp5 devices.
>>
>> /*
>> * 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)
>> return 0;
>
> This condition should be changed to check for the MDP_CLK being
> available in the clocks array rather than checking for is_mdp5. I'd
> like to phase is_mdp5 away at some point.
>
>> In that case, what prevents us from moving this to dpu and start using
>> catalog for this?
>
> Because there is nothing tying mdss and dpu drivers. For example, is
> the msm8998 (3.0.0) the DPU or MDP5 device? MSM8996?
> Neither struct msm_mdss nor the MDSS device itself are accessible
> through the msm_drv (or dpu_kms).
> I think trying to invent such a link would make the code worse.
>
Right, what I am trying to mention with that check is that means that
code does not run today for mdp5 and it still works fine.
So why not just move it to DPU first to carry less burden of these extra
register settings which are unused today for mdp5 anyway.
>>>> /* struct dpu_mdp_cfg : MDP TOP-BLK instance info
>>>> * @id: index identifying this block
>>>> * @base: register base offset to mdss
>>>> * @features bit mask identifying sub-blocks/features
>>>> * @highest_bank_bit: UBWC parameter
>>>> * @ubwc_static: ubwc static configuration
>>>> * @ubwc_swizzle: ubwc default swizzle setting
>>>> * @clk_ctrls clock control register definition
>>>> */
>>>> struct dpu_mdp_cfg {
>>>> DPU_HW_BLK_INFO;
>>>> u32 highest_bank_bit;
>>>> u32 ubwc_swizzle;
>>>> struct dpu_clk_ctrl_reg clk_ctrls[DPU_CLK_CTRL_MAX];
>>>> };
>>>>
>>>> We already do seem to have a couple of parameters. have to add the others.
>>>>
>>>> That way the number of functions wont keep growing.
>
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Freedreno] [PATCH] drm/msm: less magic numbers in msm_mdss_enable
2022-06-08 22:35 ` Abhinav Kumar
@ 2022-06-08 22:38 ` Dmitry Baryshkov
-1 siblings, 0 replies; 32+ messages in thread
From: Dmitry Baryshkov @ 2022-06-08 22:38 UTC (permalink / raw)
To: Abhinav Kumar
Cc: freedreno, David Airlie, linux-arm-msm, dri-devel, Stephen Boyd,
Bjorn Andersson, Sean Paul
On 09/06/2022 01:35, Abhinav Kumar wrote:
>
>
> On 6/8/2022 3:30 PM, Dmitry Baryshkov wrote:
>> On Wed, 8 Jun 2022 at 22:29, Abhinav Kumar <quic_abhinavk@quicinc.com>
>> wrote:
>>>
>>>
>>>
>>> On 6/2/2022 1:13 PM, Dmitry Baryshkov wrote:
>>>> On Thu, 2 Jun 2022 at 21:18, Abhinav Kumar
>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 6/1/2022 1:04 PM, Dmitry Baryshkov wrote:
>>>>>> On Wed, 1 Jun 2022 at 20:38, Abhinav Kumar
>>>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 6/1/2022 2:46 AM, Dmitry Baryshkov wrote:
>>>>>>>> On Wed, 1 Jun 2022 at 01:01, Abhinav Kumar
>>>>>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>>>>> On 5/31/2022 5:18 AM, Dmitry Baryshkov wrote:
>>>>>>>>>> Replace magic register writes in msm_mdss_enable() with
>>>>>>>>>> version that
>>>>>>>>>> contains less magic and more variable names that can be traced
>>>>>>>>>> back to
>>>>>>>>>> the dpu_hw_catalog or the downstream dtsi files.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>>>>>> ---
>>>>>>>>>> drivers/gpu/drm/msm/msm_mdss.c | 79
>>>>>>>>>> ++++++++++++++++++++++++++++++----
>>>>>>>>>> 1 file changed, 71 insertions(+), 8 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>>>>> b/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>>>>> index 0454a571adf7..2a48263cd1b5 100644
>>>>>>>>>> --- a/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>>>>> +++ b/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>>>>> @@ -21,6 +21,7 @@
>>>>>>>>>> #define HW_REV 0x0
>>>>>>>>>> #define HW_INTR_STATUS 0x0010
>>>>>>>>>>
>>>>>>>>>> +#define UBWC_DEC_HW_VERSION 0x58
>>>>>>>>>> #define UBWC_STATIC 0x144
>>>>>>>>>> #define UBWC_CTRL_2 0x150
>>>>>>>>>> #define UBWC_PREDICTION_MODE 0x154
>>>>>>>>>> @@ -132,9 +133,63 @@ static int
>>>>>>>>>> _msm_mdss_irq_domain_add(struct msm_mdss *msm_mdss)
>>>>>>>>>> return 0;
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> +#define UBWC_1_0 0x10000000
>>>>>>>>>> +#define UBWC_2_0 0x20000000
>>>>>>>>>> +#define UBWC_3_0 0x30000000
>>>>>>>>>> +#define UBWC_4_0 0x40000000
>>>>>>>>>> +
>>>>>>>>>> +static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss
>>>>>>>>>> *msm_mdss,
>>>>>>>>>> + u32 ubwc_static)
>>>>>>>>>> +{
>>>>>>>>>> + writel_relaxed(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)
>>>>>>>>>> +{
>>>>>>>>>> + u32 value = (ubwc_swizzle & 0x1) |
>>>>>>>>>> + (highest_bank_bit & 0x3) << 4 |
>>>>>>>>>> + (macrotile_mode & 0x1) << 12;
>>>>>>>>>> +
>>>>>>>>>> + if (ubwc_version == UBWC_3_0)
>>>>>>>>>> + value |= BIT(10);
>>>>>>>>>> +
>>>>>>>>>> + if (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)
>>>>>>>>>> +{
>>>>>>>>>> + u32 value = (ubwc_swizzle & 0x7) |
>>>>>>>>>> + (ubwc_static & 0x1) << 3 |
>>>>>>>>>> + (highest_bank_bit & 0x7) << 4 |
>>>>>>>>>> + (macrotile_mode & 0x1) << 12;
>>>>>>>>>> +
>>>>>>>>>> + writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
>>>>>>>>>> +
>>>>>>>>>> + if (ubwc_version == UBWC_3_0) {
>>>>>>>>>> + writel_relaxed(1, msm_mdss->mmio + UBWC_CTRL_2);
>>>>>>>>>> + writel_relaxed(0, msm_mdss->mmio +
>>>>>>>>>> UBWC_PREDICTION_MODE);
>>>>>>>>>> + } else {
>>>>>>>>>> + writel_relaxed(2, msm_mdss->mmio + UBWC_CTRL_2);
>>>>>>>>>> + writel_relaxed(1, msm_mdss->mmio +
>>>>>>>>>> UBWC_PREDICTION_MODE);
>>>>>>>>>> + }
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>
>>>>>>>>> Is it possible to unify the above functions by having the internal
>>>>>>>>> ubwc_version checks?
>>>>>>>>
>>>>>>>> Note, it's not the ubwc_version, it is the ubwc_dec_hw_version. And
>>>>>>>> also different functions take different sets of arguments.
>>>>>>>>
>>>>>>>>> It seems like msm_mdss_setup_ubwc_dec_xxx can keep growing.
>>>>>>>>>
>>>>>>>>> I have not looked into each bit programming but from the top
>>>>>>>>> level so
>>>>>>>>> feel free to correct if wrong but it seems both do write
>>>>>>>>> UBWC_STATIC
>>>>>>>>> (different values based on different UBWC versions) and write
>>>>>>>>> some extra
>>>>>>>>> registers based on version
>>>>>>>>
>>>>>>>> This is what both the current code and the downstream do. See
>>>>>>>> https://github.com/MiCode/Xiaomi_Kernel_OpenSource/blob/zeus-s-oss/techpack/display-drivers/msm/sde/sde_hw_top.c#L312
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>> Thanks for pointing to the downstream method for this,
>>>>>>>
>>>>>>> This is exactly what i was also suggesting to do when I mentioned
>>>>>>> unifying the above functions.
>>>>>>>
>>>>>>> So instead of having a separate function for each version why not
>>>>>>> handle
>>>>>>> all the versions in the same function like what the link you have
>>>>>>> shown
>>>>>>> does.
>>>>>>
>>>>>> I wouldn't like that. The downstream uses hw_catalog to pass all
>>>>>> possible parameters. We do not, so we'd have a whole set of
>>>>>> artificial
>>>>>> values.
>>>>>>
>>>>>
>>>>> Now that you brought that up, why cannot even upstream dpu start using
>>>>> catalog for ubwc settings?
>>>>
>>>> Because msm_mdss lives out of disp/dpu1. And using the disp/dpu1 for
>>>> it would be an inversion of dependencies.
>>>> I like the fact that msm_mdss is independent of mdp/dpu drivers and I
>>>> do not want to add such dependency.
>>>>
>>>
>>> Ok, so I think this function itself is placed incorrectly. It should not
>>> be in msm_mdss.c and should in the DPU folder.
>>>
>>> This check tells me that this will not be executed for mdp5 devices.
>>>
>>> /*
>>> * 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)
>>> return 0;
>>
>> This condition should be changed to check for the MDP_CLK being
>> available in the clocks array rather than checking for is_mdp5. I'd
>> like to phase is_mdp5 away at some point.
>>
>>> In that case, what prevents us from moving this to dpu and start using
>>> catalog for this?
>>
>> Because there is nothing tying mdss and dpu drivers. For example, is
>> the msm8998 (3.0.0) the DPU or MDP5 device? MSM8996?
>> Neither struct msm_mdss nor the MDSS device itself are accessible
>> through the msm_drv (or dpu_kms).
>> I think trying to invent such a link would make the code worse.
>>
>
> Right, what I am trying to mention with that check is that means that
> code does not run today for mdp5 and it still works fine.
>
> So why not just move it to DPU first to carry less burden of these extra
> register settings which are unused today for mdp5 anyway.
As I mentioned, there is no good way. msm_mdss doesn't know about DPU.
DPU doesn't know about the msm_mdss. Even the msm_drv doesn't know about
the msm_mdss.
If you can sketch a nice piece of code, could you please demonstrate
your idea?
>
>>>>> /* struct dpu_mdp_cfg : MDP TOP-BLK instance info
>>>>> * @id: index identifying this block
>>>>> * @base: register base offset to mdss
>>>>> * @features bit mask identifying sub-blocks/features
>>>>> * @highest_bank_bit: UBWC parameter
>>>>> * @ubwc_static: ubwc static configuration
>>>>> * @ubwc_swizzle: ubwc default swizzle setting
>>>>> * @clk_ctrls clock control register definition
>>>>> */
>>>>> struct dpu_mdp_cfg {
>>>>> DPU_HW_BLK_INFO;
>>>>> u32 highest_bank_bit;
>>>>> u32 ubwc_swizzle;
>>>>> struct dpu_clk_ctrl_reg clk_ctrls[DPU_CLK_CTRL_MAX];
>>>>> };
>>>>>
>>>>> We already do seem to have a couple of parameters. have to add the
>>>>> others.
>>>>>
>>>>> That way the number of functions wont keep growing.
>>
>>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Freedreno] [PATCH] drm/msm: less magic numbers in msm_mdss_enable
@ 2022-06-08 22:38 ` Dmitry Baryshkov
0 siblings, 0 replies; 32+ messages in thread
From: Dmitry Baryshkov @ 2022-06-08 22:38 UTC (permalink / raw)
To: Abhinav Kumar
Cc: Sean Paul, David Airlie, linux-arm-msm, dri-devel,
Bjorn Andersson, Stephen Boyd, freedreno
On 09/06/2022 01:35, Abhinav Kumar wrote:
>
>
> On 6/8/2022 3:30 PM, Dmitry Baryshkov wrote:
>> On Wed, 8 Jun 2022 at 22:29, Abhinav Kumar <quic_abhinavk@quicinc.com>
>> wrote:
>>>
>>>
>>>
>>> On 6/2/2022 1:13 PM, Dmitry Baryshkov wrote:
>>>> On Thu, 2 Jun 2022 at 21:18, Abhinav Kumar
>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 6/1/2022 1:04 PM, Dmitry Baryshkov wrote:
>>>>>> On Wed, 1 Jun 2022 at 20:38, Abhinav Kumar
>>>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 6/1/2022 2:46 AM, Dmitry Baryshkov wrote:
>>>>>>>> On Wed, 1 Jun 2022 at 01:01, Abhinav Kumar
>>>>>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>>>>> On 5/31/2022 5:18 AM, Dmitry Baryshkov wrote:
>>>>>>>>>> Replace magic register writes in msm_mdss_enable() with
>>>>>>>>>> version that
>>>>>>>>>> contains less magic and more variable names that can be traced
>>>>>>>>>> back to
>>>>>>>>>> the dpu_hw_catalog or the downstream dtsi files.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>>>>>> ---
>>>>>>>>>> drivers/gpu/drm/msm/msm_mdss.c | 79
>>>>>>>>>> ++++++++++++++++++++++++++++++----
>>>>>>>>>> 1 file changed, 71 insertions(+), 8 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>>>>> b/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>>>>> index 0454a571adf7..2a48263cd1b5 100644
>>>>>>>>>> --- a/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>>>>> +++ b/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>>>>> @@ -21,6 +21,7 @@
>>>>>>>>>> #define HW_REV 0x0
>>>>>>>>>> #define HW_INTR_STATUS 0x0010
>>>>>>>>>>
>>>>>>>>>> +#define UBWC_DEC_HW_VERSION 0x58
>>>>>>>>>> #define UBWC_STATIC 0x144
>>>>>>>>>> #define UBWC_CTRL_2 0x150
>>>>>>>>>> #define UBWC_PREDICTION_MODE 0x154
>>>>>>>>>> @@ -132,9 +133,63 @@ static int
>>>>>>>>>> _msm_mdss_irq_domain_add(struct msm_mdss *msm_mdss)
>>>>>>>>>> return 0;
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> +#define UBWC_1_0 0x10000000
>>>>>>>>>> +#define UBWC_2_0 0x20000000
>>>>>>>>>> +#define UBWC_3_0 0x30000000
>>>>>>>>>> +#define UBWC_4_0 0x40000000
>>>>>>>>>> +
>>>>>>>>>> +static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss
>>>>>>>>>> *msm_mdss,
>>>>>>>>>> + u32 ubwc_static)
>>>>>>>>>> +{
>>>>>>>>>> + writel_relaxed(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)
>>>>>>>>>> +{
>>>>>>>>>> + u32 value = (ubwc_swizzle & 0x1) |
>>>>>>>>>> + (highest_bank_bit & 0x3) << 4 |
>>>>>>>>>> + (macrotile_mode & 0x1) << 12;
>>>>>>>>>> +
>>>>>>>>>> + if (ubwc_version == UBWC_3_0)
>>>>>>>>>> + value |= BIT(10);
>>>>>>>>>> +
>>>>>>>>>> + if (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)
>>>>>>>>>> +{
>>>>>>>>>> + u32 value = (ubwc_swizzle & 0x7) |
>>>>>>>>>> + (ubwc_static & 0x1) << 3 |
>>>>>>>>>> + (highest_bank_bit & 0x7) << 4 |
>>>>>>>>>> + (macrotile_mode & 0x1) << 12;
>>>>>>>>>> +
>>>>>>>>>> + writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
>>>>>>>>>> +
>>>>>>>>>> + if (ubwc_version == UBWC_3_0) {
>>>>>>>>>> + writel_relaxed(1, msm_mdss->mmio + UBWC_CTRL_2);
>>>>>>>>>> + writel_relaxed(0, msm_mdss->mmio +
>>>>>>>>>> UBWC_PREDICTION_MODE);
>>>>>>>>>> + } else {
>>>>>>>>>> + writel_relaxed(2, msm_mdss->mmio + UBWC_CTRL_2);
>>>>>>>>>> + writel_relaxed(1, msm_mdss->mmio +
>>>>>>>>>> UBWC_PREDICTION_MODE);
>>>>>>>>>> + }
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>
>>>>>>>>> Is it possible to unify the above functions by having the internal
>>>>>>>>> ubwc_version checks?
>>>>>>>>
>>>>>>>> Note, it's not the ubwc_version, it is the ubwc_dec_hw_version. And
>>>>>>>> also different functions take different sets of arguments.
>>>>>>>>
>>>>>>>>> It seems like msm_mdss_setup_ubwc_dec_xxx can keep growing.
>>>>>>>>>
>>>>>>>>> I have not looked into each bit programming but from the top
>>>>>>>>> level so
>>>>>>>>> feel free to correct if wrong but it seems both do write
>>>>>>>>> UBWC_STATIC
>>>>>>>>> (different values based on different UBWC versions) and write
>>>>>>>>> some extra
>>>>>>>>> registers based on version
>>>>>>>>
>>>>>>>> This is what both the current code and the downstream do. See
>>>>>>>> https://github.com/MiCode/Xiaomi_Kernel_OpenSource/blob/zeus-s-oss/techpack/display-drivers/msm/sde/sde_hw_top.c#L312
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>> Thanks for pointing to the downstream method for this,
>>>>>>>
>>>>>>> This is exactly what i was also suggesting to do when I mentioned
>>>>>>> unifying the above functions.
>>>>>>>
>>>>>>> So instead of having a separate function for each version why not
>>>>>>> handle
>>>>>>> all the versions in the same function like what the link you have
>>>>>>> shown
>>>>>>> does.
>>>>>>
>>>>>> I wouldn't like that. The downstream uses hw_catalog to pass all
>>>>>> possible parameters. We do not, so we'd have a whole set of
>>>>>> artificial
>>>>>> values.
>>>>>>
>>>>>
>>>>> Now that you brought that up, why cannot even upstream dpu start using
>>>>> catalog for ubwc settings?
>>>>
>>>> Because msm_mdss lives out of disp/dpu1. And using the disp/dpu1 for
>>>> it would be an inversion of dependencies.
>>>> I like the fact that msm_mdss is independent of mdp/dpu drivers and I
>>>> do not want to add such dependency.
>>>>
>>>
>>> Ok, so I think this function itself is placed incorrectly. It should not
>>> be in msm_mdss.c and should in the DPU folder.
>>>
>>> This check tells me that this will not be executed for mdp5 devices.
>>>
>>> /*
>>> * 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)
>>> return 0;
>>
>> This condition should be changed to check for the MDP_CLK being
>> available in the clocks array rather than checking for is_mdp5. I'd
>> like to phase is_mdp5 away at some point.
>>
>>> In that case, what prevents us from moving this to dpu and start using
>>> catalog for this?
>>
>> Because there is nothing tying mdss and dpu drivers. For example, is
>> the msm8998 (3.0.0) the DPU or MDP5 device? MSM8996?
>> Neither struct msm_mdss nor the MDSS device itself are accessible
>> through the msm_drv (or dpu_kms).
>> I think trying to invent such a link would make the code worse.
>>
>
> Right, what I am trying to mention with that check is that means that
> code does not run today for mdp5 and it still works fine.
>
> So why not just move it to DPU first to carry less burden of these extra
> register settings which are unused today for mdp5 anyway.
As I mentioned, there is no good way. msm_mdss doesn't know about DPU.
DPU doesn't know about the msm_mdss. Even the msm_drv doesn't know about
the msm_mdss.
If you can sketch a nice piece of code, could you please demonstrate
your idea?
>
>>>>> /* struct dpu_mdp_cfg : MDP TOP-BLK instance info
>>>>> * @id: index identifying this block
>>>>> * @base: register base offset to mdss
>>>>> * @features bit mask identifying sub-blocks/features
>>>>> * @highest_bank_bit: UBWC parameter
>>>>> * @ubwc_static: ubwc static configuration
>>>>> * @ubwc_swizzle: ubwc default swizzle setting
>>>>> * @clk_ctrls clock control register definition
>>>>> */
>>>>> struct dpu_mdp_cfg {
>>>>> DPU_HW_BLK_INFO;
>>>>> u32 highest_bank_bit;
>>>>> u32 ubwc_swizzle;
>>>>> struct dpu_clk_ctrl_reg clk_ctrls[DPU_CLK_CTRL_MAX];
>>>>> };
>>>>>
>>>>> We already do seem to have a couple of parameters. have to add the
>>>>> others.
>>>>>
>>>>> That way the number of functions wont keep growing.
>>
>>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Freedreno] [PATCH] drm/msm: less magic numbers in msm_mdss_enable
2022-06-08 22:38 ` Dmitry Baryshkov
@ 2022-06-08 22:42 ` Abhinav Kumar
-1 siblings, 0 replies; 32+ messages in thread
From: Abhinav Kumar @ 2022-06-08 22:42 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: freedreno, David Airlie, linux-arm-msm, dri-devel, Stephen Boyd,
Bjorn Andersson, Sean Paul
On 6/8/2022 3:38 PM, Dmitry Baryshkov wrote:
> On 09/06/2022 01:35, Abhinav Kumar wrote:
>>
>>
>> On 6/8/2022 3:30 PM, Dmitry Baryshkov wrote:
>>> On Wed, 8 Jun 2022 at 22:29, Abhinav Kumar
>>> <quic_abhinavk@quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 6/2/2022 1:13 PM, Dmitry Baryshkov wrote:
>>>>> On Thu, 2 Jun 2022 at 21:18, Abhinav Kumar
>>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 6/1/2022 1:04 PM, Dmitry Baryshkov wrote:
>>>>>>> On Wed, 1 Jun 2022 at 20:38, Abhinav Kumar
>>>>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 6/1/2022 2:46 AM, Dmitry Baryshkov wrote:
>>>>>>>>> On Wed, 1 Jun 2022 at 01:01, Abhinav Kumar
>>>>>>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>>>>>> On 5/31/2022 5:18 AM, Dmitry Baryshkov wrote:
>>>>>>>>>>> Replace magic register writes in msm_mdss_enable() with
>>>>>>>>>>> version that
>>>>>>>>>>> contains less magic and more variable names that can be
>>>>>>>>>>> traced back to
>>>>>>>>>>> the dpu_hw_catalog or the downstream dtsi files.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>>>>>>> ---
>>>>>>>>>>> drivers/gpu/drm/msm/msm_mdss.c | 79
>>>>>>>>>>> ++++++++++++++++++++++++++++++----
>>>>>>>>>>> 1 file changed, 71 insertions(+), 8 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>>>>>> b/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>>>>>> index 0454a571adf7..2a48263cd1b5 100644
>>>>>>>>>>> --- a/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>>>>>> +++ b/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>>>>>> @@ -21,6 +21,7 @@
>>>>>>>>>>> #define HW_REV 0x0
>>>>>>>>>>> #define HW_INTR_STATUS 0x0010
>>>>>>>>>>>
>>>>>>>>>>> +#define UBWC_DEC_HW_VERSION 0x58
>>>>>>>>>>> #define UBWC_STATIC 0x144
>>>>>>>>>>> #define UBWC_CTRL_2 0x150
>>>>>>>>>>> #define UBWC_PREDICTION_MODE 0x154
>>>>>>>>>>> @@ -132,9 +133,63 @@ static int
>>>>>>>>>>> _msm_mdss_irq_domain_add(struct msm_mdss *msm_mdss)
>>>>>>>>>>> return 0;
>>>>>>>>>>> }
>>>>>>>>>>>
>>>>>>>>>>> +#define UBWC_1_0 0x10000000
>>>>>>>>>>> +#define UBWC_2_0 0x20000000
>>>>>>>>>>> +#define UBWC_3_0 0x30000000
>>>>>>>>>>> +#define UBWC_4_0 0x40000000
>>>>>>>>>>> +
>>>>>>>>>>> +static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss
>>>>>>>>>>> *msm_mdss,
>>>>>>>>>>> + u32 ubwc_static)
>>>>>>>>>>> +{
>>>>>>>>>>> + writel_relaxed(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)
>>>>>>>>>>> +{
>>>>>>>>>>> + u32 value = (ubwc_swizzle & 0x1) |
>>>>>>>>>>> + (highest_bank_bit & 0x3) << 4 |
>>>>>>>>>>> + (macrotile_mode & 0x1) << 12;
>>>>>>>>>>> +
>>>>>>>>>>> + if (ubwc_version == UBWC_3_0)
>>>>>>>>>>> + value |= BIT(10);
>>>>>>>>>>> +
>>>>>>>>>>> + if (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)
>>>>>>>>>>> +{
>>>>>>>>>>> + u32 value = (ubwc_swizzle & 0x7) |
>>>>>>>>>>> + (ubwc_static & 0x1) << 3 |
>>>>>>>>>>> + (highest_bank_bit & 0x7) << 4 |
>>>>>>>>>>> + (macrotile_mode & 0x1) << 12;
>>>>>>>>>>> +
>>>>>>>>>>> + writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
>>>>>>>>>>> +
>>>>>>>>>>> + if (ubwc_version == UBWC_3_0) {
>>>>>>>>>>> + writel_relaxed(1, msm_mdss->mmio + UBWC_CTRL_2);
>>>>>>>>>>> + writel_relaxed(0, msm_mdss->mmio +
>>>>>>>>>>> UBWC_PREDICTION_MODE);
>>>>>>>>>>> + } else {
>>>>>>>>>>> + writel_relaxed(2, msm_mdss->mmio + UBWC_CTRL_2);
>>>>>>>>>>> + writel_relaxed(1, msm_mdss->mmio +
>>>>>>>>>>> UBWC_PREDICTION_MODE);
>>>>>>>>>>> + }
>>>>>>>>>>> +}
>>>>>>>>>>> +
>>>>>>>>>>
>>>>>>>>>> Is it possible to unify the above functions by having the
>>>>>>>>>> internal
>>>>>>>>>> ubwc_version checks?
>>>>>>>>>
>>>>>>>>> Note, it's not the ubwc_version, it is the ubwc_dec_hw_version.
>>>>>>>>> And
>>>>>>>>> also different functions take different sets of arguments.
>>>>>>>>>
>>>>>>>>>> It seems like msm_mdss_setup_ubwc_dec_xxx can keep growing.
>>>>>>>>>>
>>>>>>>>>> I have not looked into each bit programming but from the top
>>>>>>>>>> level so
>>>>>>>>>> feel free to correct if wrong but it seems both do write
>>>>>>>>>> UBWC_STATIC
>>>>>>>>>> (different values based on different UBWC versions) and write
>>>>>>>>>> some extra
>>>>>>>>>> registers based on version
>>>>>>>>>
>>>>>>>>> This is what both the current code and the downstream do. See
>>>>>>>>> https://github.com/MiCode/Xiaomi_Kernel_OpenSource/blob/zeus-s-oss/techpack/display-drivers/msm/sde/sde_hw_top.c#L312
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>> Thanks for pointing to the downstream method for this,
>>>>>>>>
>>>>>>>> This is exactly what i was also suggesting to do when I mentioned
>>>>>>>> unifying the above functions.
>>>>>>>>
>>>>>>>> So instead of having a separate function for each version why
>>>>>>>> not handle
>>>>>>>> all the versions in the same function like what the link you
>>>>>>>> have shown
>>>>>>>> does.
>>>>>>>
>>>>>>> I wouldn't like that. The downstream uses hw_catalog to pass all
>>>>>>> possible parameters. We do not, so we'd have a whole set of
>>>>>>> artificial
>>>>>>> values.
>>>>>>>
>>>>>>
>>>>>> Now that you brought that up, why cannot even upstream dpu start
>>>>>> using
>>>>>> catalog for ubwc settings?
>>>>>
>>>>> Because msm_mdss lives out of disp/dpu1. And using the disp/dpu1 for
>>>>> it would be an inversion of dependencies.
>>>>> I like the fact that msm_mdss is independent of mdp/dpu drivers and I
>>>>> do not want to add such dependency.
>>>>>
>>>>
>>>> Ok, so I think this function itself is placed incorrectly. It should
>>>> not
>>>> be in msm_mdss.c and should in the DPU folder.
>>>>
>>>> This check tells me that this will not be executed for mdp5 devices.
>>>>
>>>> /*
>>>> * 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)
>>>> return 0;
>>>
>>> This condition should be changed to check for the MDP_CLK being
>>> available in the clocks array rather than checking for is_mdp5. I'd
>>> like to phase is_mdp5 away at some point.
>>>
>>>> In that case, what prevents us from moving this to dpu and start using
>>>> catalog for this?
>>>
>>> Because there is nothing tying mdss and dpu drivers. For example, is
>>> the msm8998 (3.0.0) the DPU or MDP5 device? MSM8996?
>>> Neither struct msm_mdss nor the MDSS device itself are accessible
>>> through the msm_drv (or dpu_kms).
>>> I think trying to invent such a link would make the code worse.
>>>
>>
>> Right, what I am trying to mention with that check is that means that
>> code does not run today for mdp5 and it still works fine.
>>
>> So why not just move it to DPU first to carry less burden of these
>> extra register settings which are unused today for mdp5 anyway.
>
> As I mentioned, there is no good way. msm_mdss doesn't know about DPU.
> DPU doesn't know about the msm_mdss. Even the msm_drv doesn't know about
> the msm_mdss.
>
> If you can sketch a nice piece of code, could you please demonstrate
> your idea?
>
No, so I am not suggesting to do it in msm_mdss. Only then you will need
msm_mdss to have knowledge of whether its DPU or MDP5.
Correct me if wrong. msm_mdss is common to both MDP5 and DPU.
From the above check its clear that this code does not run for MDP5.
So I am suggesting move this code completely to dpu_runtime_resume().
That way you can use catalog there.
I am questioning why we even need this function to be in msm_mdss. It
can just belong in DPU as its not being used by MDP5 today.
>>
>>>>>> /* struct dpu_mdp_cfg : MDP TOP-BLK instance info
>>>>>> * @id: index identifying this block
>>>>>> * @base: register base offset to mdss
>>>>>> * @features bit mask identifying sub-blocks/features
>>>>>> * @highest_bank_bit: UBWC parameter
>>>>>> * @ubwc_static: ubwc static configuration
>>>>>> * @ubwc_swizzle: ubwc default swizzle setting
>>>>>> * @clk_ctrls clock control register definition
>>>>>> */
>>>>>> struct dpu_mdp_cfg {
>>>>>> DPU_HW_BLK_INFO;
>>>>>> u32 highest_bank_bit;
>>>>>> u32 ubwc_swizzle;
>>>>>> struct dpu_clk_ctrl_reg clk_ctrls[DPU_CLK_CTRL_MAX];
>>>>>> };
>>>>>>
>>>>>> We already do seem to have a couple of parameters. have to add the
>>>>>> others.
>>>>>>
>>>>>> That way the number of functions wont keep growing.
>>>
>>>
>
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Freedreno] [PATCH] drm/msm: less magic numbers in msm_mdss_enable
@ 2022-06-08 22:42 ` Abhinav Kumar
0 siblings, 0 replies; 32+ messages in thread
From: Abhinav Kumar @ 2022-06-08 22:42 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Sean Paul, David Airlie, linux-arm-msm, dri-devel,
Bjorn Andersson, Stephen Boyd, freedreno
On 6/8/2022 3:38 PM, Dmitry Baryshkov wrote:
> On 09/06/2022 01:35, Abhinav Kumar wrote:
>>
>>
>> On 6/8/2022 3:30 PM, Dmitry Baryshkov wrote:
>>> On Wed, 8 Jun 2022 at 22:29, Abhinav Kumar
>>> <quic_abhinavk@quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 6/2/2022 1:13 PM, Dmitry Baryshkov wrote:
>>>>> On Thu, 2 Jun 2022 at 21:18, Abhinav Kumar
>>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 6/1/2022 1:04 PM, Dmitry Baryshkov wrote:
>>>>>>> On Wed, 1 Jun 2022 at 20:38, Abhinav Kumar
>>>>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 6/1/2022 2:46 AM, Dmitry Baryshkov wrote:
>>>>>>>>> On Wed, 1 Jun 2022 at 01:01, Abhinav Kumar
>>>>>>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>>>>>> On 5/31/2022 5:18 AM, Dmitry Baryshkov wrote:
>>>>>>>>>>> Replace magic register writes in msm_mdss_enable() with
>>>>>>>>>>> version that
>>>>>>>>>>> contains less magic and more variable names that can be
>>>>>>>>>>> traced back to
>>>>>>>>>>> the dpu_hw_catalog or the downstream dtsi files.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>>>>>>> ---
>>>>>>>>>>> drivers/gpu/drm/msm/msm_mdss.c | 79
>>>>>>>>>>> ++++++++++++++++++++++++++++++----
>>>>>>>>>>> 1 file changed, 71 insertions(+), 8 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>>>>>> b/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>>>>>> index 0454a571adf7..2a48263cd1b5 100644
>>>>>>>>>>> --- a/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>>>>>> +++ b/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>>>>>> @@ -21,6 +21,7 @@
>>>>>>>>>>> #define HW_REV 0x0
>>>>>>>>>>> #define HW_INTR_STATUS 0x0010
>>>>>>>>>>>
>>>>>>>>>>> +#define UBWC_DEC_HW_VERSION 0x58
>>>>>>>>>>> #define UBWC_STATIC 0x144
>>>>>>>>>>> #define UBWC_CTRL_2 0x150
>>>>>>>>>>> #define UBWC_PREDICTION_MODE 0x154
>>>>>>>>>>> @@ -132,9 +133,63 @@ static int
>>>>>>>>>>> _msm_mdss_irq_domain_add(struct msm_mdss *msm_mdss)
>>>>>>>>>>> return 0;
>>>>>>>>>>> }
>>>>>>>>>>>
>>>>>>>>>>> +#define UBWC_1_0 0x10000000
>>>>>>>>>>> +#define UBWC_2_0 0x20000000
>>>>>>>>>>> +#define UBWC_3_0 0x30000000
>>>>>>>>>>> +#define UBWC_4_0 0x40000000
>>>>>>>>>>> +
>>>>>>>>>>> +static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss
>>>>>>>>>>> *msm_mdss,
>>>>>>>>>>> + u32 ubwc_static)
>>>>>>>>>>> +{
>>>>>>>>>>> + writel_relaxed(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)
>>>>>>>>>>> +{
>>>>>>>>>>> + u32 value = (ubwc_swizzle & 0x1) |
>>>>>>>>>>> + (highest_bank_bit & 0x3) << 4 |
>>>>>>>>>>> + (macrotile_mode & 0x1) << 12;
>>>>>>>>>>> +
>>>>>>>>>>> + if (ubwc_version == UBWC_3_0)
>>>>>>>>>>> + value |= BIT(10);
>>>>>>>>>>> +
>>>>>>>>>>> + if (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)
>>>>>>>>>>> +{
>>>>>>>>>>> + u32 value = (ubwc_swizzle & 0x7) |
>>>>>>>>>>> + (ubwc_static & 0x1) << 3 |
>>>>>>>>>>> + (highest_bank_bit & 0x7) << 4 |
>>>>>>>>>>> + (macrotile_mode & 0x1) << 12;
>>>>>>>>>>> +
>>>>>>>>>>> + writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
>>>>>>>>>>> +
>>>>>>>>>>> + if (ubwc_version == UBWC_3_0) {
>>>>>>>>>>> + writel_relaxed(1, msm_mdss->mmio + UBWC_CTRL_2);
>>>>>>>>>>> + writel_relaxed(0, msm_mdss->mmio +
>>>>>>>>>>> UBWC_PREDICTION_MODE);
>>>>>>>>>>> + } else {
>>>>>>>>>>> + writel_relaxed(2, msm_mdss->mmio + UBWC_CTRL_2);
>>>>>>>>>>> + writel_relaxed(1, msm_mdss->mmio +
>>>>>>>>>>> UBWC_PREDICTION_MODE);
>>>>>>>>>>> + }
>>>>>>>>>>> +}
>>>>>>>>>>> +
>>>>>>>>>>
>>>>>>>>>> Is it possible to unify the above functions by having the
>>>>>>>>>> internal
>>>>>>>>>> ubwc_version checks?
>>>>>>>>>
>>>>>>>>> Note, it's not the ubwc_version, it is the ubwc_dec_hw_version.
>>>>>>>>> And
>>>>>>>>> also different functions take different sets of arguments.
>>>>>>>>>
>>>>>>>>>> It seems like msm_mdss_setup_ubwc_dec_xxx can keep growing.
>>>>>>>>>>
>>>>>>>>>> I have not looked into each bit programming but from the top
>>>>>>>>>> level so
>>>>>>>>>> feel free to correct if wrong but it seems both do write
>>>>>>>>>> UBWC_STATIC
>>>>>>>>>> (different values based on different UBWC versions) and write
>>>>>>>>>> some extra
>>>>>>>>>> registers based on version
>>>>>>>>>
>>>>>>>>> This is what both the current code and the downstream do. See
>>>>>>>>> https://github.com/MiCode/Xiaomi_Kernel_OpenSource/blob/zeus-s-oss/techpack/display-drivers/msm/sde/sde_hw_top.c#L312
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>> Thanks for pointing to the downstream method for this,
>>>>>>>>
>>>>>>>> This is exactly what i was also suggesting to do when I mentioned
>>>>>>>> unifying the above functions.
>>>>>>>>
>>>>>>>> So instead of having a separate function for each version why
>>>>>>>> not handle
>>>>>>>> all the versions in the same function like what the link you
>>>>>>>> have shown
>>>>>>>> does.
>>>>>>>
>>>>>>> I wouldn't like that. The downstream uses hw_catalog to pass all
>>>>>>> possible parameters. We do not, so we'd have a whole set of
>>>>>>> artificial
>>>>>>> values.
>>>>>>>
>>>>>>
>>>>>> Now that you brought that up, why cannot even upstream dpu start
>>>>>> using
>>>>>> catalog for ubwc settings?
>>>>>
>>>>> Because msm_mdss lives out of disp/dpu1. And using the disp/dpu1 for
>>>>> it would be an inversion of dependencies.
>>>>> I like the fact that msm_mdss is independent of mdp/dpu drivers and I
>>>>> do not want to add such dependency.
>>>>>
>>>>
>>>> Ok, so I think this function itself is placed incorrectly. It should
>>>> not
>>>> be in msm_mdss.c and should in the DPU folder.
>>>>
>>>> This check tells me that this will not be executed for mdp5 devices.
>>>>
>>>> /*
>>>> * 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)
>>>> return 0;
>>>
>>> This condition should be changed to check for the MDP_CLK being
>>> available in the clocks array rather than checking for is_mdp5. I'd
>>> like to phase is_mdp5 away at some point.
>>>
>>>> In that case, what prevents us from moving this to dpu and start using
>>>> catalog for this?
>>>
>>> Because there is nothing tying mdss and dpu drivers. For example, is
>>> the msm8998 (3.0.0) the DPU or MDP5 device? MSM8996?
>>> Neither struct msm_mdss nor the MDSS device itself are accessible
>>> through the msm_drv (or dpu_kms).
>>> I think trying to invent such a link would make the code worse.
>>>
>>
>> Right, what I am trying to mention with that check is that means that
>> code does not run today for mdp5 and it still works fine.
>>
>> So why not just move it to DPU first to carry less burden of these
>> extra register settings which are unused today for mdp5 anyway.
>
> As I mentioned, there is no good way. msm_mdss doesn't know about DPU.
> DPU doesn't know about the msm_mdss. Even the msm_drv doesn't know about
> the msm_mdss.
>
> If you can sketch a nice piece of code, could you please demonstrate
> your idea?
>
No, so I am not suggesting to do it in msm_mdss. Only then you will need
msm_mdss to have knowledge of whether its DPU or MDP5.
Correct me if wrong. msm_mdss is common to both MDP5 and DPU.
From the above check its clear that this code does not run for MDP5.
So I am suggesting move this code completely to dpu_runtime_resume().
That way you can use catalog there.
I am questioning why we even need this function to be in msm_mdss. It
can just belong in DPU as its not being used by MDP5 today.
>>
>>>>>> /* struct dpu_mdp_cfg : MDP TOP-BLK instance info
>>>>>> * @id: index identifying this block
>>>>>> * @base: register base offset to mdss
>>>>>> * @features bit mask identifying sub-blocks/features
>>>>>> * @highest_bank_bit: UBWC parameter
>>>>>> * @ubwc_static: ubwc static configuration
>>>>>> * @ubwc_swizzle: ubwc default swizzle setting
>>>>>> * @clk_ctrls clock control register definition
>>>>>> */
>>>>>> struct dpu_mdp_cfg {
>>>>>> DPU_HW_BLK_INFO;
>>>>>> u32 highest_bank_bit;
>>>>>> u32 ubwc_swizzle;
>>>>>> struct dpu_clk_ctrl_reg clk_ctrls[DPU_CLK_CTRL_MAX];
>>>>>> };
>>>>>>
>>>>>> We already do seem to have a couple of parameters. have to add the
>>>>>> others.
>>>>>>
>>>>>> That way the number of functions wont keep growing.
>>>
>>>
>
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Freedreno] [PATCH] drm/msm: less magic numbers in msm_mdss_enable
2022-06-08 22:42 ` Abhinav Kumar
@ 2022-06-08 22:46 ` Dmitry Baryshkov
-1 siblings, 0 replies; 32+ messages in thread
From: Dmitry Baryshkov @ 2022-06-08 22:46 UTC (permalink / raw)
To: Abhinav Kumar
Cc: freedreno, David Airlie, linux-arm-msm, dri-devel, Stephen Boyd,
Bjorn Andersson, Sean Paul
On 09/06/2022 01:42, Abhinav Kumar wrote:
>
>
> On 6/8/2022 3:38 PM, Dmitry Baryshkov wrote:
>> On 09/06/2022 01:35, Abhinav Kumar wrote:
>>>
>>>
>>> On 6/8/2022 3:30 PM, Dmitry Baryshkov wrote:
>>>> On Wed, 8 Jun 2022 at 22:29, Abhinav Kumar
>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 6/2/2022 1:13 PM, Dmitry Baryshkov wrote:
>>>>>> On Thu, 2 Jun 2022 at 21:18, Abhinav Kumar
>>>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 6/1/2022 1:04 PM, Dmitry Baryshkov wrote:
>>>>>>>> On Wed, 1 Jun 2022 at 20:38, Abhinav Kumar
>>>>>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 6/1/2022 2:46 AM, Dmitry Baryshkov wrote:
>>>>>>>>>> On Wed, 1 Jun 2022 at 01:01, Abhinav Kumar
>>>>>>>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>>>>>>> On 5/31/2022 5:18 AM, Dmitry Baryshkov wrote:
>>>>>>>>>>>> Replace magic register writes in msm_mdss_enable() with
>>>>>>>>>>>> version that
>>>>>>>>>>>> contains less magic and more variable names that can be
>>>>>>>>>>>> traced back to
>>>>>>>>>>>> the dpu_hw_catalog or the downstream dtsi files.
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>>>>>>>> ---
>>>>>>>>>>>> drivers/gpu/drm/msm/msm_mdss.c | 79
>>>>>>>>>>>> ++++++++++++++++++++++++++++++----
>>>>>>>>>>>> 1 file changed, 71 insertions(+), 8 deletions(-)
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>>>>>>> b/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>>>>>>> index 0454a571adf7..2a48263cd1b5 100644
>>>>>>>>>>>> --- a/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>>>>>>> +++ b/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>>>>>>> @@ -21,6 +21,7 @@
>>>>>>>>>>>> #define HW_REV 0x0
>>>>>>>>>>>> #define HW_INTR_STATUS 0x0010
>>>>>>>>>>>>
>>>>>>>>>>>> +#define UBWC_DEC_HW_VERSION 0x58
>>>>>>>>>>>> #define UBWC_STATIC 0x144
>>>>>>>>>>>> #define UBWC_CTRL_2 0x150
>>>>>>>>>>>> #define UBWC_PREDICTION_MODE 0x154
>>>>>>>>>>>> @@ -132,9 +133,63 @@ static int
>>>>>>>>>>>> _msm_mdss_irq_domain_add(struct msm_mdss *msm_mdss)
>>>>>>>>>>>> return 0;
>>>>>>>>>>>> }
>>>>>>>>>>>>
>>>>>>>>>>>> +#define UBWC_1_0 0x10000000
>>>>>>>>>>>> +#define UBWC_2_0 0x20000000
>>>>>>>>>>>> +#define UBWC_3_0 0x30000000
>>>>>>>>>>>> +#define UBWC_4_0 0x40000000
>>>>>>>>>>>> +
>>>>>>>>>>>> +static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss
>>>>>>>>>>>> *msm_mdss,
>>>>>>>>>>>> + u32 ubwc_static)
>>>>>>>>>>>> +{
>>>>>>>>>>>> + writel_relaxed(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)
>>>>>>>>>>>> +{
>>>>>>>>>>>> + u32 value = (ubwc_swizzle & 0x1) |
>>>>>>>>>>>> + (highest_bank_bit & 0x3) << 4 |
>>>>>>>>>>>> + (macrotile_mode & 0x1) << 12;
>>>>>>>>>>>> +
>>>>>>>>>>>> + if (ubwc_version == UBWC_3_0)
>>>>>>>>>>>> + value |= BIT(10);
>>>>>>>>>>>> +
>>>>>>>>>>>> + if (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)
>>>>>>>>>>>> +{
>>>>>>>>>>>> + u32 value = (ubwc_swizzle & 0x7) |
>>>>>>>>>>>> + (ubwc_static & 0x1) << 3 |
>>>>>>>>>>>> + (highest_bank_bit & 0x7) << 4 |
>>>>>>>>>>>> + (macrotile_mode & 0x1) << 12;
>>>>>>>>>>>> +
>>>>>>>>>>>> + writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
>>>>>>>>>>>> +
>>>>>>>>>>>> + if (ubwc_version == UBWC_3_0) {
>>>>>>>>>>>> + writel_relaxed(1, msm_mdss->mmio + UBWC_CTRL_2);
>>>>>>>>>>>> + writel_relaxed(0, msm_mdss->mmio +
>>>>>>>>>>>> UBWC_PREDICTION_MODE);
>>>>>>>>>>>> + } else {
>>>>>>>>>>>> + writel_relaxed(2, msm_mdss->mmio + UBWC_CTRL_2);
>>>>>>>>>>>> + writel_relaxed(1, msm_mdss->mmio +
>>>>>>>>>>>> UBWC_PREDICTION_MODE);
>>>>>>>>>>>> + }
>>>>>>>>>>>> +}
>>>>>>>>>>>> +
>>>>>>>>>>>
>>>>>>>>>>> Is it possible to unify the above functions by having the
>>>>>>>>>>> internal
>>>>>>>>>>> ubwc_version checks?
>>>>>>>>>>
>>>>>>>>>> Note, it's not the ubwc_version, it is the
>>>>>>>>>> ubwc_dec_hw_version. And
>>>>>>>>>> also different functions take different sets of arguments.
>>>>>>>>>>
>>>>>>>>>>> It seems like msm_mdss_setup_ubwc_dec_xxx can keep growing.
>>>>>>>>>>>
>>>>>>>>>>> I have not looked into each bit programming but from the top
>>>>>>>>>>> level so
>>>>>>>>>>> feel free to correct if wrong but it seems both do write
>>>>>>>>>>> UBWC_STATIC
>>>>>>>>>>> (different values based on different UBWC versions) and write
>>>>>>>>>>> some extra
>>>>>>>>>>> registers based on version
>>>>>>>>>>
>>>>>>>>>> This is what both the current code and the downstream do. See
>>>>>>>>>> https://github.com/MiCode/Xiaomi_Kernel_OpenSource/blob/zeus-s-oss/techpack/display-drivers/msm/sde/sde_hw_top.c#L312
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thanks for pointing to the downstream method for this,
>>>>>>>>>
>>>>>>>>> This is exactly what i was also suggesting to do when I mentioned
>>>>>>>>> unifying the above functions.
>>>>>>>>>
>>>>>>>>> So instead of having a separate function for each version why
>>>>>>>>> not handle
>>>>>>>>> all the versions in the same function like what the link you
>>>>>>>>> have shown
>>>>>>>>> does.
>>>>>>>>
>>>>>>>> I wouldn't like that. The downstream uses hw_catalog to pass all
>>>>>>>> possible parameters. We do not, so we'd have a whole set of
>>>>>>>> artificial
>>>>>>>> values.
>>>>>>>>
>>>>>>>
>>>>>>> Now that you brought that up, why cannot even upstream dpu start
>>>>>>> using
>>>>>>> catalog for ubwc settings?
>>>>>>
>>>>>> Because msm_mdss lives out of disp/dpu1. And using the disp/dpu1 for
>>>>>> it would be an inversion of dependencies.
>>>>>> I like the fact that msm_mdss is independent of mdp/dpu drivers and I
>>>>>> do not want to add such dependency.
>>>>>>
>>>>>
>>>>> Ok, so I think this function itself is placed incorrectly. It
>>>>> should not
>>>>> be in msm_mdss.c and should in the DPU folder.
>>>>>
>>>>> This check tells me that this will not be executed for mdp5 devices.
>>>>>
>>>>> /*
>>>>> * 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)
>>>>> return 0;
>>>>
>>>> This condition should be changed to check for the MDP_CLK being
>>>> available in the clocks array rather than checking for is_mdp5. I'd
>>>> like to phase is_mdp5 away at some point.
>>>>
>>>>> In that case, what prevents us from moving this to dpu and start using
>>>>> catalog for this?
>>>>
>>>> Because there is nothing tying mdss and dpu drivers. For example, is
>>>> the msm8998 (3.0.0) the DPU or MDP5 device? MSM8996?
>>>> Neither struct msm_mdss nor the MDSS device itself are accessible
>>>> through the msm_drv (or dpu_kms).
>>>> I think trying to invent such a link would make the code worse.
>>>>
>>>
>>> Right, what I am trying to mention with that check is that means that
>>> code does not run today for mdp5 and it still works fine.
>>>
>>> So why not just move it to DPU first to carry less burden of these
>>> extra register settings which are unused today for mdp5 anyway.
>>
>> As I mentioned, there is no good way. msm_mdss doesn't know about DPU.
>> DPU doesn't know about the msm_mdss. Even the msm_drv doesn't know
>> about the msm_mdss.
>>
>> If you can sketch a nice piece of code, could you please demonstrate
>> your idea?
>>
>
> No, so I am not suggesting to do it in msm_mdss. Only then you will need
> msm_mdss to have knowledge of whether its DPU or MDP5.
>
> Correct me if wrong. msm_mdss is common to both MDP5 and DPU.
>
> From the above check its clear that this code does not run for MDP5.
>
> So I am suggesting move this code completely to dpu_runtime_resume().
>
> That way you can use catalog there.
>
> I am questioning why we even need this function to be in msm_mdss. It
> can just belong in DPU as its not being used by MDP5 today.
The region used by the mdss is not mapped by the DPU (or MDP5). This
caused some confusion with DPU code trying to write to the non-mapped or
incorrect memory areas before we ended up with Jonathan fixing the code
in 544d8b96150d ("drm/msm/dpu: update UBWC config for sm8150 and sm8250").
>
>>>
>>>>>>> /* struct dpu_mdp_cfg : MDP TOP-BLK instance info
>>>>>>> * @id: index identifying this block
>>>>>>> * @base: register base offset to mdss
>>>>>>> * @features bit mask identifying sub-blocks/features
>>>>>>> * @highest_bank_bit: UBWC parameter
>>>>>>> * @ubwc_static: ubwc static configuration
>>>>>>> * @ubwc_swizzle: ubwc default swizzle setting
>>>>>>> * @clk_ctrls clock control register definition
>>>>>>> */
>>>>>>> struct dpu_mdp_cfg {
>>>>>>> DPU_HW_BLK_INFO;
>>>>>>> u32 highest_bank_bit;
>>>>>>> u32 ubwc_swizzle;
>>>>>>> struct dpu_clk_ctrl_reg clk_ctrls[DPU_CLK_CTRL_MAX];
>>>>>>> };
>>>>>>>
>>>>>>> We already do seem to have a couple of parameters. have to add
>>>>>>> the others.
>>>>>>>
>>>>>>> That way the number of functions wont keep growing.
>>>>
>>>>
>>
>>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Freedreno] [PATCH] drm/msm: less magic numbers in msm_mdss_enable
@ 2022-06-08 22:46 ` Dmitry Baryshkov
0 siblings, 0 replies; 32+ messages in thread
From: Dmitry Baryshkov @ 2022-06-08 22:46 UTC (permalink / raw)
To: Abhinav Kumar
Cc: Sean Paul, David Airlie, linux-arm-msm, dri-devel,
Bjorn Andersson, Stephen Boyd, freedreno
On 09/06/2022 01:42, Abhinav Kumar wrote:
>
>
> On 6/8/2022 3:38 PM, Dmitry Baryshkov wrote:
>> On 09/06/2022 01:35, Abhinav Kumar wrote:
>>>
>>>
>>> On 6/8/2022 3:30 PM, Dmitry Baryshkov wrote:
>>>> On Wed, 8 Jun 2022 at 22:29, Abhinav Kumar
>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 6/2/2022 1:13 PM, Dmitry Baryshkov wrote:
>>>>>> On Thu, 2 Jun 2022 at 21:18, Abhinav Kumar
>>>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 6/1/2022 1:04 PM, Dmitry Baryshkov wrote:
>>>>>>>> On Wed, 1 Jun 2022 at 20:38, Abhinav Kumar
>>>>>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 6/1/2022 2:46 AM, Dmitry Baryshkov wrote:
>>>>>>>>>> On Wed, 1 Jun 2022 at 01:01, Abhinav Kumar
>>>>>>>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>>>>>>> On 5/31/2022 5:18 AM, Dmitry Baryshkov wrote:
>>>>>>>>>>>> Replace magic register writes in msm_mdss_enable() with
>>>>>>>>>>>> version that
>>>>>>>>>>>> contains less magic and more variable names that can be
>>>>>>>>>>>> traced back to
>>>>>>>>>>>> the dpu_hw_catalog or the downstream dtsi files.
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>>>>>>>> ---
>>>>>>>>>>>> drivers/gpu/drm/msm/msm_mdss.c | 79
>>>>>>>>>>>> ++++++++++++++++++++++++++++++----
>>>>>>>>>>>> 1 file changed, 71 insertions(+), 8 deletions(-)
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>>>>>>> b/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>>>>>>> index 0454a571adf7..2a48263cd1b5 100644
>>>>>>>>>>>> --- a/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>>>>>>> +++ b/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>>>>>>> @@ -21,6 +21,7 @@
>>>>>>>>>>>> #define HW_REV 0x0
>>>>>>>>>>>> #define HW_INTR_STATUS 0x0010
>>>>>>>>>>>>
>>>>>>>>>>>> +#define UBWC_DEC_HW_VERSION 0x58
>>>>>>>>>>>> #define UBWC_STATIC 0x144
>>>>>>>>>>>> #define UBWC_CTRL_2 0x150
>>>>>>>>>>>> #define UBWC_PREDICTION_MODE 0x154
>>>>>>>>>>>> @@ -132,9 +133,63 @@ static int
>>>>>>>>>>>> _msm_mdss_irq_domain_add(struct msm_mdss *msm_mdss)
>>>>>>>>>>>> return 0;
>>>>>>>>>>>> }
>>>>>>>>>>>>
>>>>>>>>>>>> +#define UBWC_1_0 0x10000000
>>>>>>>>>>>> +#define UBWC_2_0 0x20000000
>>>>>>>>>>>> +#define UBWC_3_0 0x30000000
>>>>>>>>>>>> +#define UBWC_4_0 0x40000000
>>>>>>>>>>>> +
>>>>>>>>>>>> +static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss
>>>>>>>>>>>> *msm_mdss,
>>>>>>>>>>>> + u32 ubwc_static)
>>>>>>>>>>>> +{
>>>>>>>>>>>> + writel_relaxed(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)
>>>>>>>>>>>> +{
>>>>>>>>>>>> + u32 value = (ubwc_swizzle & 0x1) |
>>>>>>>>>>>> + (highest_bank_bit & 0x3) << 4 |
>>>>>>>>>>>> + (macrotile_mode & 0x1) << 12;
>>>>>>>>>>>> +
>>>>>>>>>>>> + if (ubwc_version == UBWC_3_0)
>>>>>>>>>>>> + value |= BIT(10);
>>>>>>>>>>>> +
>>>>>>>>>>>> + if (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)
>>>>>>>>>>>> +{
>>>>>>>>>>>> + u32 value = (ubwc_swizzle & 0x7) |
>>>>>>>>>>>> + (ubwc_static & 0x1) << 3 |
>>>>>>>>>>>> + (highest_bank_bit & 0x7) << 4 |
>>>>>>>>>>>> + (macrotile_mode & 0x1) << 12;
>>>>>>>>>>>> +
>>>>>>>>>>>> + writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
>>>>>>>>>>>> +
>>>>>>>>>>>> + if (ubwc_version == UBWC_3_0) {
>>>>>>>>>>>> + writel_relaxed(1, msm_mdss->mmio + UBWC_CTRL_2);
>>>>>>>>>>>> + writel_relaxed(0, msm_mdss->mmio +
>>>>>>>>>>>> UBWC_PREDICTION_MODE);
>>>>>>>>>>>> + } else {
>>>>>>>>>>>> + writel_relaxed(2, msm_mdss->mmio + UBWC_CTRL_2);
>>>>>>>>>>>> + writel_relaxed(1, msm_mdss->mmio +
>>>>>>>>>>>> UBWC_PREDICTION_MODE);
>>>>>>>>>>>> + }
>>>>>>>>>>>> +}
>>>>>>>>>>>> +
>>>>>>>>>>>
>>>>>>>>>>> Is it possible to unify the above functions by having the
>>>>>>>>>>> internal
>>>>>>>>>>> ubwc_version checks?
>>>>>>>>>>
>>>>>>>>>> Note, it's not the ubwc_version, it is the
>>>>>>>>>> ubwc_dec_hw_version. And
>>>>>>>>>> also different functions take different sets of arguments.
>>>>>>>>>>
>>>>>>>>>>> It seems like msm_mdss_setup_ubwc_dec_xxx can keep growing.
>>>>>>>>>>>
>>>>>>>>>>> I have not looked into each bit programming but from the top
>>>>>>>>>>> level so
>>>>>>>>>>> feel free to correct if wrong but it seems both do write
>>>>>>>>>>> UBWC_STATIC
>>>>>>>>>>> (different values based on different UBWC versions) and write
>>>>>>>>>>> some extra
>>>>>>>>>>> registers based on version
>>>>>>>>>>
>>>>>>>>>> This is what both the current code and the downstream do. See
>>>>>>>>>> https://github.com/MiCode/Xiaomi_Kernel_OpenSource/blob/zeus-s-oss/techpack/display-drivers/msm/sde/sde_hw_top.c#L312
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thanks for pointing to the downstream method for this,
>>>>>>>>>
>>>>>>>>> This is exactly what i was also suggesting to do when I mentioned
>>>>>>>>> unifying the above functions.
>>>>>>>>>
>>>>>>>>> So instead of having a separate function for each version why
>>>>>>>>> not handle
>>>>>>>>> all the versions in the same function like what the link you
>>>>>>>>> have shown
>>>>>>>>> does.
>>>>>>>>
>>>>>>>> I wouldn't like that. The downstream uses hw_catalog to pass all
>>>>>>>> possible parameters. We do not, so we'd have a whole set of
>>>>>>>> artificial
>>>>>>>> values.
>>>>>>>>
>>>>>>>
>>>>>>> Now that you brought that up, why cannot even upstream dpu start
>>>>>>> using
>>>>>>> catalog for ubwc settings?
>>>>>>
>>>>>> Because msm_mdss lives out of disp/dpu1. And using the disp/dpu1 for
>>>>>> it would be an inversion of dependencies.
>>>>>> I like the fact that msm_mdss is independent of mdp/dpu drivers and I
>>>>>> do not want to add such dependency.
>>>>>>
>>>>>
>>>>> Ok, so I think this function itself is placed incorrectly. It
>>>>> should not
>>>>> be in msm_mdss.c and should in the DPU folder.
>>>>>
>>>>> This check tells me that this will not be executed for mdp5 devices.
>>>>>
>>>>> /*
>>>>> * 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)
>>>>> return 0;
>>>>
>>>> This condition should be changed to check for the MDP_CLK being
>>>> available in the clocks array rather than checking for is_mdp5. I'd
>>>> like to phase is_mdp5 away at some point.
>>>>
>>>>> In that case, what prevents us from moving this to dpu and start using
>>>>> catalog for this?
>>>>
>>>> Because there is nothing tying mdss and dpu drivers. For example, is
>>>> the msm8998 (3.0.0) the DPU or MDP5 device? MSM8996?
>>>> Neither struct msm_mdss nor the MDSS device itself are accessible
>>>> through the msm_drv (or dpu_kms).
>>>> I think trying to invent such a link would make the code worse.
>>>>
>>>
>>> Right, what I am trying to mention with that check is that means that
>>> code does not run today for mdp5 and it still works fine.
>>>
>>> So why not just move it to DPU first to carry less burden of these
>>> extra register settings which are unused today for mdp5 anyway.
>>
>> As I mentioned, there is no good way. msm_mdss doesn't know about DPU.
>> DPU doesn't know about the msm_mdss. Even the msm_drv doesn't know
>> about the msm_mdss.
>>
>> If you can sketch a nice piece of code, could you please demonstrate
>> your idea?
>>
>
> No, so I am not suggesting to do it in msm_mdss. Only then you will need
> msm_mdss to have knowledge of whether its DPU or MDP5.
>
> Correct me if wrong. msm_mdss is common to both MDP5 and DPU.
>
> From the above check its clear that this code does not run for MDP5.
>
> So I am suggesting move this code completely to dpu_runtime_resume().
>
> That way you can use catalog there.
>
> I am questioning why we even need this function to be in msm_mdss. It
> can just belong in DPU as its not being used by MDP5 today.
The region used by the mdss is not mapped by the DPU (or MDP5). This
caused some confusion with DPU code trying to write to the non-mapped or
incorrect memory areas before we ended up with Jonathan fixing the code
in 544d8b96150d ("drm/msm/dpu: update UBWC config for sm8150 and sm8250").
>
>>>
>>>>>>> /* struct dpu_mdp_cfg : MDP TOP-BLK instance info
>>>>>>> * @id: index identifying this block
>>>>>>> * @base: register base offset to mdss
>>>>>>> * @features bit mask identifying sub-blocks/features
>>>>>>> * @highest_bank_bit: UBWC parameter
>>>>>>> * @ubwc_static: ubwc static configuration
>>>>>>> * @ubwc_swizzle: ubwc default swizzle setting
>>>>>>> * @clk_ctrls clock control register definition
>>>>>>> */
>>>>>>> struct dpu_mdp_cfg {
>>>>>>> DPU_HW_BLK_INFO;
>>>>>>> u32 highest_bank_bit;
>>>>>>> u32 ubwc_swizzle;
>>>>>>> struct dpu_clk_ctrl_reg clk_ctrls[DPU_CLK_CTRL_MAX];
>>>>>>> };
>>>>>>>
>>>>>>> We already do seem to have a couple of parameters. have to add
>>>>>>> the others.
>>>>>>>
>>>>>>> That way the number of functions wont keep growing.
>>>>
>>>>
>>
>>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Freedreno] [PATCH] drm/msm: less magic numbers in msm_mdss_enable
2022-06-08 22:46 ` Dmitry Baryshkov
@ 2022-06-08 23:37 ` Abhinav Kumar
-1 siblings, 0 replies; 32+ messages in thread
From: Abhinav Kumar @ 2022-06-08 23:37 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Sean Paul, David Airlie, linux-arm-msm, dri-devel,
Bjorn Andersson, Stephen Boyd, freedreno
On 6/8/2022 3:46 PM, Dmitry Baryshkov wrote:
> On 09/06/2022 01:42, Abhinav Kumar wrote:
>>
>>
>> On 6/8/2022 3:38 PM, Dmitry Baryshkov wrote:
>>> On 09/06/2022 01:35, Abhinav Kumar wrote:
>>>>
>>>>
>>>> On 6/8/2022 3:30 PM, Dmitry Baryshkov wrote:
>>>>> On Wed, 8 Jun 2022 at 22:29, Abhinav Kumar
>>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 6/2/2022 1:13 PM, Dmitry Baryshkov wrote:
>>>>>>> On Thu, 2 Jun 2022 at 21:18, Abhinav Kumar
>>>>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 6/1/2022 1:04 PM, Dmitry Baryshkov wrote:
>>>>>>>>> On Wed, 1 Jun 2022 at 20:38, Abhinav Kumar
>>>>>>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 6/1/2022 2:46 AM, Dmitry Baryshkov wrote:
>>>>>>>>>>> On Wed, 1 Jun 2022 at 01:01, Abhinav Kumar
>>>>>>>>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>>>>>>>> On 5/31/2022 5:18 AM, Dmitry Baryshkov wrote:
>>>>>>>>>>>>> Replace magic register writes in msm_mdss_enable() with
>>>>>>>>>>>>> version that
>>>>>>>>>>>>> contains less magic and more variable names that can be
>>>>>>>>>>>>> traced back to
>>>>>>>>>>>>> the dpu_hw_catalog or the downstream dtsi files.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>> drivers/gpu/drm/msm/msm_mdss.c | 79
>>>>>>>>>>>>> ++++++++++++++++++++++++++++++----
>>>>>>>>>>>>> 1 file changed, 71 insertions(+), 8 deletions(-)
>>>>>>>>>>>>>
>>>>>>>>>>>>> diff --git a/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>>>>>>>> b/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>>>>>>>> index 0454a571adf7..2a48263cd1b5 100644
>>>>>>>>>>>>> --- a/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>>>>>>>> +++ b/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>>>>>>>> @@ -21,6 +21,7 @@
>>>>>>>>>>>>> #define HW_REV 0x0
>>>>>>>>>>>>> #define HW_INTR_STATUS 0x0010
>>>>>>>>>>>>>
>>>>>>>>>>>>> +#define UBWC_DEC_HW_VERSION 0x58
>>>>>>>>>>>>> #define UBWC_STATIC 0x144
>>>>>>>>>>>>> #define UBWC_CTRL_2 0x150
>>>>>>>>>>>>> #define UBWC_PREDICTION_MODE 0x154
>>>>>>>>>>>>> @@ -132,9 +133,63 @@ static int
>>>>>>>>>>>>> _msm_mdss_irq_domain_add(struct msm_mdss *msm_mdss)
>>>>>>>>>>>>> return 0;
>>>>>>>>>>>>> }
>>>>>>>>>>>>>
>>>>>>>>>>>>> +#define UBWC_1_0 0x10000000
>>>>>>>>>>>>> +#define UBWC_2_0 0x20000000
>>>>>>>>>>>>> +#define UBWC_3_0 0x30000000
>>>>>>>>>>>>> +#define UBWC_4_0 0x40000000
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss
>>>>>>>>>>>>> *msm_mdss,
>>>>>>>>>>>>> + u32 ubwc_static)
>>>>>>>>>>>>> +{
>>>>>>>>>>>>> + writel_relaxed(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)
>>>>>>>>>>>>> +{
>>>>>>>>>>>>> + u32 value = (ubwc_swizzle & 0x1) |
>>>>>>>>>>>>> + (highest_bank_bit & 0x3) << 4 |
>>>>>>>>>>>>> + (macrotile_mode & 0x1) << 12;
>>>>>>>>>>>>> +
>>>>>>>>>>>>> + if (ubwc_version == UBWC_3_0)
>>>>>>>>>>>>> + value |= BIT(10);
>>>>>>>>>>>>> +
>>>>>>>>>>>>> + if (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)
>>>>>>>>>>>>> +{
>>>>>>>>>>>>> + u32 value = (ubwc_swizzle & 0x7) |
>>>>>>>>>>>>> + (ubwc_static & 0x1) << 3 |
>>>>>>>>>>>>> + (highest_bank_bit & 0x7) << 4 |
>>>>>>>>>>>>> + (macrotile_mode & 0x1) << 12;
>>>>>>>>>>>>> +
>>>>>>>>>>>>> + writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
>>>>>>>>>>>>> +
>>>>>>>>>>>>> + if (ubwc_version == UBWC_3_0) {
>>>>>>>>>>>>> + writel_relaxed(1, msm_mdss->mmio + UBWC_CTRL_2);
>>>>>>>>>>>>> + writel_relaxed(0, msm_mdss->mmio +
>>>>>>>>>>>>> UBWC_PREDICTION_MODE);
>>>>>>>>>>>>> + } else {
>>>>>>>>>>>>> + writel_relaxed(2, msm_mdss->mmio + UBWC_CTRL_2);
>>>>>>>>>>>>> + writel_relaxed(1, msm_mdss->mmio +
>>>>>>>>>>>>> UBWC_PREDICTION_MODE);
>>>>>>>>>>>>> + }
>>>>>>>>>>>>> +}
>>>>>>>>>>>>> +
>>>>>>>>>>>>
>>>>>>>>>>>> Is it possible to unify the above functions by having the
>>>>>>>>>>>> internal
>>>>>>>>>>>> ubwc_version checks?
>>>>>>>>>>>
>>>>>>>>>>> Note, it's not the ubwc_version, it is the
>>>>>>>>>>> ubwc_dec_hw_version. And
>>>>>>>>>>> also different functions take different sets of arguments.
>>>>>>>>>>>
>>>>>>>>>>>> It seems like msm_mdss_setup_ubwc_dec_xxx can keep growing.
>>>>>>>>>>>>
>>>>>>>>>>>> I have not looked into each bit programming but from the top
>>>>>>>>>>>> level so
>>>>>>>>>>>> feel free to correct if wrong but it seems both do write
>>>>>>>>>>>> UBWC_STATIC
>>>>>>>>>>>> (different values based on different UBWC versions) and
>>>>>>>>>>>> write some extra
>>>>>>>>>>>> registers based on version
>>>>>>>>>>>
>>>>>>>>>>> This is what both the current code and the downstream do. See
>>>>>>>>>>> https://github.com/MiCode/Xiaomi_Kernel_OpenSource/blob/zeus-s-oss/techpack/display-drivers/msm/sde/sde_hw_top.c#L312
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Thanks for pointing to the downstream method for this,
>>>>>>>>>>
>>>>>>>>>> This is exactly what i was also suggesting to do when I mentioned
>>>>>>>>>> unifying the above functions.
>>>>>>>>>>
>>>>>>>>>> So instead of having a separate function for each version why
>>>>>>>>>> not handle
>>>>>>>>>> all the versions in the same function like what the link you
>>>>>>>>>> have shown
>>>>>>>>>> does.
>>>>>>>>>
>>>>>>>>> I wouldn't like that. The downstream uses hw_catalog to pass all
>>>>>>>>> possible parameters. We do not, so we'd have a whole set of
>>>>>>>>> artificial
>>>>>>>>> values.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Now that you brought that up, why cannot even upstream dpu start
>>>>>>>> using
>>>>>>>> catalog for ubwc settings?
>>>>>>>
>>>>>>> Because msm_mdss lives out of disp/dpu1. And using the disp/dpu1 for
>>>>>>> it would be an inversion of dependencies.
>>>>>>> I like the fact that msm_mdss is independent of mdp/dpu drivers
>>>>>>> and I
>>>>>>> do not want to add such dependency.
>>>>>>>
>>>>>>
>>>>>> Ok, so I think this function itself is placed incorrectly. It
>>>>>> should not
>>>>>> be in msm_mdss.c and should in the DPU folder.
>>>>>>
>>>>>> This check tells me that this will not be executed for mdp5 devices.
>>>>>>
>>>>>> /*
>>>>>> * 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)
>>>>>> return 0;
>>>>>
>>>>> This condition should be changed to check for the MDP_CLK being
>>>>> available in the clocks array rather than checking for is_mdp5. I'd
>>>>> like to phase is_mdp5 away at some point.
>>>>>
>>>>>> In that case, what prevents us from moving this to dpu and start
>>>>>> using
>>>>>> catalog for this?
>>>>>
>>>>> Because there is nothing tying mdss and dpu drivers. For example, is
>>>>> the msm8998 (3.0.0) the DPU or MDP5 device? MSM8996?
>>>>> Neither struct msm_mdss nor the MDSS device itself are accessible
>>>>> through the msm_drv (or dpu_kms).
>>>>> I think trying to invent such a link would make the code worse.
>>>>>
>>>>
>>>> Right, what I am trying to mention with that check is that means
>>>> that code does not run today for mdp5 and it still works fine.
>>>>
>>>> So why not just move it to DPU first to carry less burden of these
>>>> extra register settings which are unused today for mdp5 anyway.
>>>
>>> As I mentioned, there is no good way. msm_mdss doesn't know about
>>> DPU. DPU doesn't know about the msm_mdss. Even the msm_drv doesn't
>>> know about the msm_mdss.
>>>
>>> If you can sketch a nice piece of code, could you please demonstrate
>>> your idea?
>>>
>>
>> No, so I am not suggesting to do it in msm_mdss. Only then you will
>> need msm_mdss to have knowledge of whether its DPU or MDP5.
>>
>> Correct me if wrong. msm_mdss is common to both MDP5 and DPU.
>>
>> From the above check its clear that this code does not run for MDP5.
>>
>> So I am suggesting move this code completely to dpu_runtime_resume().
>>
>> That way you can use catalog there.
>>
>> I am questioning why we even need this function to be in msm_mdss. It
>> can just belong in DPU as its not being used by MDP5 today.
>
> The region used by the mdss is not mapped by the DPU (or MDP5). This
> caused some confusion with DPU code trying to write to the non-mapped or
> incorrect memory areas before we ended up with Jonathan fixing the code
> in 544d8b96150d ("drm/msm/dpu: update UBWC config for sm8150 and sm8250").
>
Thanks so much for the providing this change , it gave me some more context.
I briefly went through this change.
So what happened was dpu_kms->mmio is different from dpu_mdss->mmio in
terms of offset.
dpu_mdss->mmio is the one which starts from 0 and thats the one we
should use for the UBWC config.
So even after Jonathan's change we were doing this in dpu_mdss.c
After we removed the dpu_mdss.c layer to start using msm_mdss.c, now
this function looks somewhat misplaced to me because it was in dpu.
One suggestion I have is, let dpu_kms.c map this region "mdss" by
getting it from its parent using of_get_parent() and store it as
"mdss_mmio". Then move this back to dpu_runtime_resume().
Then rework that function to remove these magic numbers and start using
catalog.
Let me know what you think. Thats the best i can come up with in the
current design.
>>
>>>>
>>>>>>>> /* struct dpu_mdp_cfg : MDP TOP-BLK instance info
>>>>>>>> * @id: index identifying this block
>>>>>>>> * @base: register base offset to mdss
>>>>>>>> * @features bit mask identifying sub-blocks/features
>>>>>>>> * @highest_bank_bit: UBWC parameter
>>>>>>>> * @ubwc_static: ubwc static configuration
>>>>>>>> * @ubwc_swizzle: ubwc default swizzle setting
>>>>>>>> * @clk_ctrls clock control register definition
>>>>>>>> */
>>>>>>>> struct dpu_mdp_cfg {
>>>>>>>> DPU_HW_BLK_INFO;
>>>>>>>> u32 highest_bank_bit;
>>>>>>>> u32 ubwc_swizzle;
>>>>>>>> struct dpu_clk_ctrl_reg clk_ctrls[DPU_CLK_CTRL_MAX];
>>>>>>>> };
>>>>>>>>
>>>>>>>> We already do seem to have a couple of parameters. have to add
>>>>>>>> the others.
>>>>>>>>
>>>>>>>> That way the number of functions wont keep growing.
>>>>>
>>>>>
>>>
>>>
>
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Freedreno] [PATCH] drm/msm: less magic numbers in msm_mdss_enable
@ 2022-06-08 23:37 ` Abhinav Kumar
0 siblings, 0 replies; 32+ messages in thread
From: Abhinav Kumar @ 2022-06-08 23:37 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: freedreno, David Airlie, linux-arm-msm, dri-devel, Stephen Boyd,
Bjorn Andersson, Sean Paul
On 6/8/2022 3:46 PM, Dmitry Baryshkov wrote:
> On 09/06/2022 01:42, Abhinav Kumar wrote:
>>
>>
>> On 6/8/2022 3:38 PM, Dmitry Baryshkov wrote:
>>> On 09/06/2022 01:35, Abhinav Kumar wrote:
>>>>
>>>>
>>>> On 6/8/2022 3:30 PM, Dmitry Baryshkov wrote:
>>>>> On Wed, 8 Jun 2022 at 22:29, Abhinav Kumar
>>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 6/2/2022 1:13 PM, Dmitry Baryshkov wrote:
>>>>>>> On Thu, 2 Jun 2022 at 21:18, Abhinav Kumar
>>>>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 6/1/2022 1:04 PM, Dmitry Baryshkov wrote:
>>>>>>>>> On Wed, 1 Jun 2022 at 20:38, Abhinav Kumar
>>>>>>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 6/1/2022 2:46 AM, Dmitry Baryshkov wrote:
>>>>>>>>>>> On Wed, 1 Jun 2022 at 01:01, Abhinav Kumar
>>>>>>>>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>>>>>>>> On 5/31/2022 5:18 AM, Dmitry Baryshkov wrote:
>>>>>>>>>>>>> Replace magic register writes in msm_mdss_enable() with
>>>>>>>>>>>>> version that
>>>>>>>>>>>>> contains less magic and more variable names that can be
>>>>>>>>>>>>> traced back to
>>>>>>>>>>>>> the dpu_hw_catalog or the downstream dtsi files.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>> drivers/gpu/drm/msm/msm_mdss.c | 79
>>>>>>>>>>>>> ++++++++++++++++++++++++++++++----
>>>>>>>>>>>>> 1 file changed, 71 insertions(+), 8 deletions(-)
>>>>>>>>>>>>>
>>>>>>>>>>>>> diff --git a/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>>>>>>>> b/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>>>>>>>> index 0454a571adf7..2a48263cd1b5 100644
>>>>>>>>>>>>> --- a/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>>>>>>>> +++ b/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>>>>>>>> @@ -21,6 +21,7 @@
>>>>>>>>>>>>> #define HW_REV 0x0
>>>>>>>>>>>>> #define HW_INTR_STATUS 0x0010
>>>>>>>>>>>>>
>>>>>>>>>>>>> +#define UBWC_DEC_HW_VERSION 0x58
>>>>>>>>>>>>> #define UBWC_STATIC 0x144
>>>>>>>>>>>>> #define UBWC_CTRL_2 0x150
>>>>>>>>>>>>> #define UBWC_PREDICTION_MODE 0x154
>>>>>>>>>>>>> @@ -132,9 +133,63 @@ static int
>>>>>>>>>>>>> _msm_mdss_irq_domain_add(struct msm_mdss *msm_mdss)
>>>>>>>>>>>>> return 0;
>>>>>>>>>>>>> }
>>>>>>>>>>>>>
>>>>>>>>>>>>> +#define UBWC_1_0 0x10000000
>>>>>>>>>>>>> +#define UBWC_2_0 0x20000000
>>>>>>>>>>>>> +#define UBWC_3_0 0x30000000
>>>>>>>>>>>>> +#define UBWC_4_0 0x40000000
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss
>>>>>>>>>>>>> *msm_mdss,
>>>>>>>>>>>>> + u32 ubwc_static)
>>>>>>>>>>>>> +{
>>>>>>>>>>>>> + writel_relaxed(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)
>>>>>>>>>>>>> +{
>>>>>>>>>>>>> + u32 value = (ubwc_swizzle & 0x1) |
>>>>>>>>>>>>> + (highest_bank_bit & 0x3) << 4 |
>>>>>>>>>>>>> + (macrotile_mode & 0x1) << 12;
>>>>>>>>>>>>> +
>>>>>>>>>>>>> + if (ubwc_version == UBWC_3_0)
>>>>>>>>>>>>> + value |= BIT(10);
>>>>>>>>>>>>> +
>>>>>>>>>>>>> + if (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)
>>>>>>>>>>>>> +{
>>>>>>>>>>>>> + u32 value = (ubwc_swizzle & 0x7) |
>>>>>>>>>>>>> + (ubwc_static & 0x1) << 3 |
>>>>>>>>>>>>> + (highest_bank_bit & 0x7) << 4 |
>>>>>>>>>>>>> + (macrotile_mode & 0x1) << 12;
>>>>>>>>>>>>> +
>>>>>>>>>>>>> + writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
>>>>>>>>>>>>> +
>>>>>>>>>>>>> + if (ubwc_version == UBWC_3_0) {
>>>>>>>>>>>>> + writel_relaxed(1, msm_mdss->mmio + UBWC_CTRL_2);
>>>>>>>>>>>>> + writel_relaxed(0, msm_mdss->mmio +
>>>>>>>>>>>>> UBWC_PREDICTION_MODE);
>>>>>>>>>>>>> + } else {
>>>>>>>>>>>>> + writel_relaxed(2, msm_mdss->mmio + UBWC_CTRL_2);
>>>>>>>>>>>>> + writel_relaxed(1, msm_mdss->mmio +
>>>>>>>>>>>>> UBWC_PREDICTION_MODE);
>>>>>>>>>>>>> + }
>>>>>>>>>>>>> +}
>>>>>>>>>>>>> +
>>>>>>>>>>>>
>>>>>>>>>>>> Is it possible to unify the above functions by having the
>>>>>>>>>>>> internal
>>>>>>>>>>>> ubwc_version checks?
>>>>>>>>>>>
>>>>>>>>>>> Note, it's not the ubwc_version, it is the
>>>>>>>>>>> ubwc_dec_hw_version. And
>>>>>>>>>>> also different functions take different sets of arguments.
>>>>>>>>>>>
>>>>>>>>>>>> It seems like msm_mdss_setup_ubwc_dec_xxx can keep growing.
>>>>>>>>>>>>
>>>>>>>>>>>> I have not looked into each bit programming but from the top
>>>>>>>>>>>> level so
>>>>>>>>>>>> feel free to correct if wrong but it seems both do write
>>>>>>>>>>>> UBWC_STATIC
>>>>>>>>>>>> (different values based on different UBWC versions) and
>>>>>>>>>>>> write some extra
>>>>>>>>>>>> registers based on version
>>>>>>>>>>>
>>>>>>>>>>> This is what both the current code and the downstream do. See
>>>>>>>>>>> https://github.com/MiCode/Xiaomi_Kernel_OpenSource/blob/zeus-s-oss/techpack/display-drivers/msm/sde/sde_hw_top.c#L312
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Thanks for pointing to the downstream method for this,
>>>>>>>>>>
>>>>>>>>>> This is exactly what i was also suggesting to do when I mentioned
>>>>>>>>>> unifying the above functions.
>>>>>>>>>>
>>>>>>>>>> So instead of having a separate function for each version why
>>>>>>>>>> not handle
>>>>>>>>>> all the versions in the same function like what the link you
>>>>>>>>>> have shown
>>>>>>>>>> does.
>>>>>>>>>
>>>>>>>>> I wouldn't like that. The downstream uses hw_catalog to pass all
>>>>>>>>> possible parameters. We do not, so we'd have a whole set of
>>>>>>>>> artificial
>>>>>>>>> values.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Now that you brought that up, why cannot even upstream dpu start
>>>>>>>> using
>>>>>>>> catalog for ubwc settings?
>>>>>>>
>>>>>>> Because msm_mdss lives out of disp/dpu1. And using the disp/dpu1 for
>>>>>>> it would be an inversion of dependencies.
>>>>>>> I like the fact that msm_mdss is independent of mdp/dpu drivers
>>>>>>> and I
>>>>>>> do not want to add such dependency.
>>>>>>>
>>>>>>
>>>>>> Ok, so I think this function itself is placed incorrectly. It
>>>>>> should not
>>>>>> be in msm_mdss.c and should in the DPU folder.
>>>>>>
>>>>>> This check tells me that this will not be executed for mdp5 devices.
>>>>>>
>>>>>> /*
>>>>>> * 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)
>>>>>> return 0;
>>>>>
>>>>> This condition should be changed to check for the MDP_CLK being
>>>>> available in the clocks array rather than checking for is_mdp5. I'd
>>>>> like to phase is_mdp5 away at some point.
>>>>>
>>>>>> In that case, what prevents us from moving this to dpu and start
>>>>>> using
>>>>>> catalog for this?
>>>>>
>>>>> Because there is nothing tying mdss and dpu drivers. For example, is
>>>>> the msm8998 (3.0.0) the DPU or MDP5 device? MSM8996?
>>>>> Neither struct msm_mdss nor the MDSS device itself are accessible
>>>>> through the msm_drv (or dpu_kms).
>>>>> I think trying to invent such a link would make the code worse.
>>>>>
>>>>
>>>> Right, what I am trying to mention with that check is that means
>>>> that code does not run today for mdp5 and it still works fine.
>>>>
>>>> So why not just move it to DPU first to carry less burden of these
>>>> extra register settings which are unused today for mdp5 anyway.
>>>
>>> As I mentioned, there is no good way. msm_mdss doesn't know about
>>> DPU. DPU doesn't know about the msm_mdss. Even the msm_drv doesn't
>>> know about the msm_mdss.
>>>
>>> If you can sketch a nice piece of code, could you please demonstrate
>>> your idea?
>>>
>>
>> No, so I am not suggesting to do it in msm_mdss. Only then you will
>> need msm_mdss to have knowledge of whether its DPU or MDP5.
>>
>> Correct me if wrong. msm_mdss is common to both MDP5 and DPU.
>>
>> From the above check its clear that this code does not run for MDP5.
>>
>> So I am suggesting move this code completely to dpu_runtime_resume().
>>
>> That way you can use catalog there.
>>
>> I am questioning why we even need this function to be in msm_mdss. It
>> can just belong in DPU as its not being used by MDP5 today.
>
> The region used by the mdss is not mapped by the DPU (or MDP5). This
> caused some confusion with DPU code trying to write to the non-mapped or
> incorrect memory areas before we ended up with Jonathan fixing the code
> in 544d8b96150d ("drm/msm/dpu: update UBWC config for sm8150 and sm8250").
>
Thanks so much for the providing this change , it gave me some more context.
I briefly went through this change.
So what happened was dpu_kms->mmio is different from dpu_mdss->mmio in
terms of offset.
dpu_mdss->mmio is the one which starts from 0 and thats the one we
should use for the UBWC config.
So even after Jonathan's change we were doing this in dpu_mdss.c
After we removed the dpu_mdss.c layer to start using msm_mdss.c, now
this function looks somewhat misplaced to me because it was in dpu.
One suggestion I have is, let dpu_kms.c map this region "mdss" by
getting it from its parent using of_get_parent() and store it as
"mdss_mmio". Then move this back to dpu_runtime_resume().
Then rework that function to remove these magic numbers and start using
catalog.
Let me know what you think. Thats the best i can come up with in the
current design.
>>
>>>>
>>>>>>>> /* struct dpu_mdp_cfg : MDP TOP-BLK instance info
>>>>>>>> * @id: index identifying this block
>>>>>>>> * @base: register base offset to mdss
>>>>>>>> * @features bit mask identifying sub-blocks/features
>>>>>>>> * @highest_bank_bit: UBWC parameter
>>>>>>>> * @ubwc_static: ubwc static configuration
>>>>>>>> * @ubwc_swizzle: ubwc default swizzle setting
>>>>>>>> * @clk_ctrls clock control register definition
>>>>>>>> */
>>>>>>>> struct dpu_mdp_cfg {
>>>>>>>> DPU_HW_BLK_INFO;
>>>>>>>> u32 highest_bank_bit;
>>>>>>>> u32 ubwc_swizzle;
>>>>>>>> struct dpu_clk_ctrl_reg clk_ctrls[DPU_CLK_CTRL_MAX];
>>>>>>>> };
>>>>>>>>
>>>>>>>> We already do seem to have a couple of parameters. have to add
>>>>>>>> the others.
>>>>>>>>
>>>>>>>> That way the number of functions wont keep growing.
>>>>>
>>>>>
>>>
>>>
>
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Freedreno] [PATCH] drm/msm: less magic numbers in msm_mdss_enable
2022-06-08 23:37 ` Abhinav Kumar
@ 2022-06-09 6:08 ` Dmitry Baryshkov
-1 siblings, 0 replies; 32+ messages in thread
From: Dmitry Baryshkov @ 2022-06-09 6:08 UTC (permalink / raw)
To: Abhinav Kumar
Cc: Sean Paul, David Airlie, linux-arm-msm, dri-devel,
Bjorn Andersson, Stephen Boyd, freedreno
On Thu, 9 Jun 2022 at 02:37, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 6/8/2022 3:46 PM, Dmitry Baryshkov wrote:
> > On 09/06/2022 01:42, Abhinav Kumar wrote:
> >>
> >>
> >> On 6/8/2022 3:38 PM, Dmitry Baryshkov wrote:
> >>> On 09/06/2022 01:35, Abhinav Kumar wrote:
> >>>>
> >>>>
> >>>> On 6/8/2022 3:30 PM, Dmitry Baryshkov wrote:
> >>>>> On Wed, 8 Jun 2022 at 22:29, Abhinav Kumar
> >>>>> <quic_abhinavk@quicinc.com> wrote:
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> On 6/2/2022 1:13 PM, Dmitry Baryshkov wrote:
> >>>>>>> On Thu, 2 Jun 2022 at 21:18, Abhinav Kumar
> >>>>>>> <quic_abhinavk@quicinc.com> wrote:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> On 6/1/2022 1:04 PM, Dmitry Baryshkov wrote:
> >>>>>>>>> On Wed, 1 Jun 2022 at 20:38, Abhinav Kumar
> >>>>>>>>> <quic_abhinavk@quicinc.com> wrote:
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> On 6/1/2022 2:46 AM, Dmitry Baryshkov wrote:
> >>>>>>>>>>> On Wed, 1 Jun 2022 at 01:01, Abhinav Kumar
> >>>>>>>>>>> <quic_abhinavk@quicinc.com> wrote:
> >>>>>>>>>>>> On 5/31/2022 5:18 AM, Dmitry Baryshkov wrote:
> >>>>>>>>>>>>> Replace magic register writes in msm_mdss_enable() with
> >>>>>>>>>>>>> version that
> >>>>>>>>>>>>> contains less magic and more variable names that can be
> >>>>>>>>>>>>> traced back to
> >>>>>>>>>>>>> the dpu_hw_catalog or the downstream dtsi files.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >>>>>>>>>>>>> ---
> >>>>>>>>>>>>> drivers/gpu/drm/msm/msm_mdss.c | 79
> >>>>>>>>>>>>> ++++++++++++++++++++++++++++++----
> >>>>>>>>>>>>> 1 file changed, 71 insertions(+), 8 deletions(-)
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> diff --git a/drivers/gpu/drm/msm/msm_mdss.c
> >>>>>>>>>>>>> b/drivers/gpu/drm/msm/msm_mdss.c
> >>>>>>>>>>>>> index 0454a571adf7..2a48263cd1b5 100644
> >>>>>>>>>>>>> --- a/drivers/gpu/drm/msm/msm_mdss.c
> >>>>>>>>>>>>> +++ b/drivers/gpu/drm/msm/msm_mdss.c
> >>>>>>>>>>>>> @@ -21,6 +21,7 @@
> >>>>>>>>>>>>> #define HW_REV 0x0
> >>>>>>>>>>>>> #define HW_INTR_STATUS 0x0010
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> +#define UBWC_DEC_HW_VERSION 0x58
> >>>>>>>>>>>>> #define UBWC_STATIC 0x144
> >>>>>>>>>>>>> #define UBWC_CTRL_2 0x150
> >>>>>>>>>>>>> #define UBWC_PREDICTION_MODE 0x154
> >>>>>>>>>>>>> @@ -132,9 +133,63 @@ static int
> >>>>>>>>>>>>> _msm_mdss_irq_domain_add(struct msm_mdss *msm_mdss)
> >>>>>>>>>>>>> return 0;
> >>>>>>>>>>>>> }
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> +#define UBWC_1_0 0x10000000
> >>>>>>>>>>>>> +#define UBWC_2_0 0x20000000
> >>>>>>>>>>>>> +#define UBWC_3_0 0x30000000
> >>>>>>>>>>>>> +#define UBWC_4_0 0x40000000
> >>>>>>>>>>>>> +
> >>>>>>>>>>>>> +static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss
> >>>>>>>>>>>>> *msm_mdss,
> >>>>>>>>>>>>> + u32 ubwc_static)
> >>>>>>>>>>>>> +{
> >>>>>>>>>>>>> + writel_relaxed(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)
> >>>>>>>>>>>>> +{
> >>>>>>>>>>>>> + u32 value = (ubwc_swizzle & 0x1) |
> >>>>>>>>>>>>> + (highest_bank_bit & 0x3) << 4 |
> >>>>>>>>>>>>> + (macrotile_mode & 0x1) << 12;
> >>>>>>>>>>>>> +
> >>>>>>>>>>>>> + if (ubwc_version == UBWC_3_0)
> >>>>>>>>>>>>> + value |= BIT(10);
> >>>>>>>>>>>>> +
> >>>>>>>>>>>>> + if (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)
> >>>>>>>>>>>>> +{
> >>>>>>>>>>>>> + u32 value = (ubwc_swizzle & 0x7) |
> >>>>>>>>>>>>> + (ubwc_static & 0x1) << 3 |
> >>>>>>>>>>>>> + (highest_bank_bit & 0x7) << 4 |
> >>>>>>>>>>>>> + (macrotile_mode & 0x1) << 12;
> >>>>>>>>>>>>> +
> >>>>>>>>>>>>> + writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
> >>>>>>>>>>>>> +
> >>>>>>>>>>>>> + if (ubwc_version == UBWC_3_0) {
> >>>>>>>>>>>>> + writel_relaxed(1, msm_mdss->mmio + UBWC_CTRL_2);
> >>>>>>>>>>>>> + writel_relaxed(0, msm_mdss->mmio +
> >>>>>>>>>>>>> UBWC_PREDICTION_MODE);
> >>>>>>>>>>>>> + } else {
> >>>>>>>>>>>>> + writel_relaxed(2, msm_mdss->mmio + UBWC_CTRL_2);
> >>>>>>>>>>>>> + writel_relaxed(1, msm_mdss->mmio +
> >>>>>>>>>>>>> UBWC_PREDICTION_MODE);
> >>>>>>>>>>>>> + }
> >>>>>>>>>>>>> +}
> >>>>>>>>>>>>> +
> >>>>>>>>>>>>
> >>>>>>>>>>>> Is it possible to unify the above functions by having the
> >>>>>>>>>>>> internal
> >>>>>>>>>>>> ubwc_version checks?
> >>>>>>>>>>>
> >>>>>>>>>>> Note, it's not the ubwc_version, it is the
> >>>>>>>>>>> ubwc_dec_hw_version. And
> >>>>>>>>>>> also different functions take different sets of arguments.
> >>>>>>>>>>>
> >>>>>>>>>>>> It seems like msm_mdss_setup_ubwc_dec_xxx can keep growing.
> >>>>>>>>>>>>
> >>>>>>>>>>>> I have not looked into each bit programming but from the top
> >>>>>>>>>>>> level so
> >>>>>>>>>>>> feel free to correct if wrong but it seems both do write
> >>>>>>>>>>>> UBWC_STATIC
> >>>>>>>>>>>> (different values based on different UBWC versions) and
> >>>>>>>>>>>> write some extra
> >>>>>>>>>>>> registers based on version
> >>>>>>>>>>>
> >>>>>>>>>>> This is what both the current code and the downstream do. See
> >>>>>>>>>>> https://github.com/MiCode/Xiaomi_Kernel_OpenSource/blob/zeus-s-oss/techpack/display-drivers/msm/sde/sde_hw_top.c#L312
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> Thanks for pointing to the downstream method for this,
> >>>>>>>>>>
> >>>>>>>>>> This is exactly what i was also suggesting to do when I mentioned
> >>>>>>>>>> unifying the above functions.
> >>>>>>>>>>
> >>>>>>>>>> So instead of having a separate function for each version why
> >>>>>>>>>> not handle
> >>>>>>>>>> all the versions in the same function like what the link you
> >>>>>>>>>> have shown
> >>>>>>>>>> does.
> >>>>>>>>>
> >>>>>>>>> I wouldn't like that. The downstream uses hw_catalog to pass all
> >>>>>>>>> possible parameters. We do not, so we'd have a whole set of
> >>>>>>>>> artificial
> >>>>>>>>> values.
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> Now that you brought that up, why cannot even upstream dpu start
> >>>>>>>> using
> >>>>>>>> catalog for ubwc settings?
> >>>>>>>
> >>>>>>> Because msm_mdss lives out of disp/dpu1. And using the disp/dpu1 for
> >>>>>>> it would be an inversion of dependencies.
> >>>>>>> I like the fact that msm_mdss is independent of mdp/dpu drivers
> >>>>>>> and I
> >>>>>>> do not want to add such dependency.
> >>>>>>>
> >>>>>>
> >>>>>> Ok, so I think this function itself is placed incorrectly. It
> >>>>>> should not
> >>>>>> be in msm_mdss.c and should in the DPU folder.
> >>>>>>
> >>>>>> This check tells me that this will not be executed for mdp5 devices.
> >>>>>>
> >>>>>> /*
> >>>>>> * 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)
> >>>>>> return 0;
> >>>>>
> >>>>> This condition should be changed to check for the MDP_CLK being
> >>>>> available in the clocks array rather than checking for is_mdp5. I'd
> >>>>> like to phase is_mdp5 away at some point.
> >>>>>
> >>>>>> In that case, what prevents us from moving this to dpu and start
> >>>>>> using
> >>>>>> catalog for this?
> >>>>>
> >>>>> Because there is nothing tying mdss and dpu drivers. For example, is
> >>>>> the msm8998 (3.0.0) the DPU or MDP5 device? MSM8996?
> >>>>> Neither struct msm_mdss nor the MDSS device itself are accessible
> >>>>> through the msm_drv (or dpu_kms).
> >>>>> I think trying to invent such a link would make the code worse.
> >>>>>
> >>>>
> >>>> Right, what I am trying to mention with that check is that means
> >>>> that code does not run today for mdp5 and it still works fine.
> >>>>
> >>>> So why not just move it to DPU first to carry less burden of these
> >>>> extra register settings which are unused today for mdp5 anyway.
> >>>
> >>> As I mentioned, there is no good way. msm_mdss doesn't know about
> >>> DPU. DPU doesn't know about the msm_mdss. Even the msm_drv doesn't
> >>> know about the msm_mdss.
> >>>
> >>> If you can sketch a nice piece of code, could you please demonstrate
> >>> your idea?
> >>>
> >>
> >> No, so I am not suggesting to do it in msm_mdss. Only then you will
> >> need msm_mdss to have knowledge of whether its DPU or MDP5.
> >>
> >> Correct me if wrong. msm_mdss is common to both MDP5 and DPU.
> >>
> >> From the above check its clear that this code does not run for MDP5.
> >>
> >> So I am suggesting move this code completely to dpu_runtime_resume().
> >>
> >> That way you can use catalog there.
> >>
> >> I am questioning why we even need this function to be in msm_mdss. It
> >> can just belong in DPU as its not being used by MDP5 today.
> >
> > The region used by the mdss is not mapped by the DPU (or MDP5). This
> > caused some confusion with DPU code trying to write to the non-mapped or
> > incorrect memory areas before we ended up with Jonathan fixing the code
> > in 544d8b96150d ("drm/msm/dpu: update UBWC config for sm8150 and sm8250").
> >
>
> Thanks so much for the providing this change , it gave me some more context.
>
> I briefly went through this change.
>
> So what happened was dpu_kms->mmio is different from dpu_mdss->mmio in
> terms of offset.
>
> dpu_mdss->mmio is the one which starts from 0 and thats the one we
> should use for the UBWC config.
>
> So even after Jonathan's change we were doing this in dpu_mdss.c
>
> After we removed the dpu_mdss.c layer to start using msm_mdss.c, now
> this function looks somewhat misplaced to me because it was in dpu.
>
> One suggestion I have is, let dpu_kms.c map this region "mdss" by
> getting it from its parent using of_get_parent() and store it as
> "mdss_mmio". Then move this back to dpu_runtime_resume().
>
> Then rework that function to remove these magic numbers and start using
> catalog.
>
> Let me know what you think. Thats the best i can come up with in the
> current design.
I feel this is an overkill for setting just three registers. MDSS
register space is not used for anything else than handling generic
interrupts and setting the UBWC. I'd leave things as is and just
replace cryptic register writes with manageable API.
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Freedreno] [PATCH] drm/msm: less magic numbers in msm_mdss_enable
@ 2022-06-09 6:08 ` Dmitry Baryshkov
0 siblings, 0 replies; 32+ messages in thread
From: Dmitry Baryshkov @ 2022-06-09 6:08 UTC (permalink / raw)
To: Abhinav Kumar
Cc: freedreno, David Airlie, linux-arm-msm, dri-devel, Stephen Boyd,
Bjorn Andersson, Sean Paul
On Thu, 9 Jun 2022 at 02:37, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 6/8/2022 3:46 PM, Dmitry Baryshkov wrote:
> > On 09/06/2022 01:42, Abhinav Kumar wrote:
> >>
> >>
> >> On 6/8/2022 3:38 PM, Dmitry Baryshkov wrote:
> >>> On 09/06/2022 01:35, Abhinav Kumar wrote:
> >>>>
> >>>>
> >>>> On 6/8/2022 3:30 PM, Dmitry Baryshkov wrote:
> >>>>> On Wed, 8 Jun 2022 at 22:29, Abhinav Kumar
> >>>>> <quic_abhinavk@quicinc.com> wrote:
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> On 6/2/2022 1:13 PM, Dmitry Baryshkov wrote:
> >>>>>>> On Thu, 2 Jun 2022 at 21:18, Abhinav Kumar
> >>>>>>> <quic_abhinavk@quicinc.com> wrote:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> On 6/1/2022 1:04 PM, Dmitry Baryshkov wrote:
> >>>>>>>>> On Wed, 1 Jun 2022 at 20:38, Abhinav Kumar
> >>>>>>>>> <quic_abhinavk@quicinc.com> wrote:
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> On 6/1/2022 2:46 AM, Dmitry Baryshkov wrote:
> >>>>>>>>>>> On Wed, 1 Jun 2022 at 01:01, Abhinav Kumar
> >>>>>>>>>>> <quic_abhinavk@quicinc.com> wrote:
> >>>>>>>>>>>> On 5/31/2022 5:18 AM, Dmitry Baryshkov wrote:
> >>>>>>>>>>>>> Replace magic register writes in msm_mdss_enable() with
> >>>>>>>>>>>>> version that
> >>>>>>>>>>>>> contains less magic and more variable names that can be
> >>>>>>>>>>>>> traced back to
> >>>>>>>>>>>>> the dpu_hw_catalog or the downstream dtsi files.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >>>>>>>>>>>>> ---
> >>>>>>>>>>>>> drivers/gpu/drm/msm/msm_mdss.c | 79
> >>>>>>>>>>>>> ++++++++++++++++++++++++++++++----
> >>>>>>>>>>>>> 1 file changed, 71 insertions(+), 8 deletions(-)
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> diff --git a/drivers/gpu/drm/msm/msm_mdss.c
> >>>>>>>>>>>>> b/drivers/gpu/drm/msm/msm_mdss.c
> >>>>>>>>>>>>> index 0454a571adf7..2a48263cd1b5 100644
> >>>>>>>>>>>>> --- a/drivers/gpu/drm/msm/msm_mdss.c
> >>>>>>>>>>>>> +++ b/drivers/gpu/drm/msm/msm_mdss.c
> >>>>>>>>>>>>> @@ -21,6 +21,7 @@
> >>>>>>>>>>>>> #define HW_REV 0x0
> >>>>>>>>>>>>> #define HW_INTR_STATUS 0x0010
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> +#define UBWC_DEC_HW_VERSION 0x58
> >>>>>>>>>>>>> #define UBWC_STATIC 0x144
> >>>>>>>>>>>>> #define UBWC_CTRL_2 0x150
> >>>>>>>>>>>>> #define UBWC_PREDICTION_MODE 0x154
> >>>>>>>>>>>>> @@ -132,9 +133,63 @@ static int
> >>>>>>>>>>>>> _msm_mdss_irq_domain_add(struct msm_mdss *msm_mdss)
> >>>>>>>>>>>>> return 0;
> >>>>>>>>>>>>> }
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> +#define UBWC_1_0 0x10000000
> >>>>>>>>>>>>> +#define UBWC_2_0 0x20000000
> >>>>>>>>>>>>> +#define UBWC_3_0 0x30000000
> >>>>>>>>>>>>> +#define UBWC_4_0 0x40000000
> >>>>>>>>>>>>> +
> >>>>>>>>>>>>> +static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss
> >>>>>>>>>>>>> *msm_mdss,
> >>>>>>>>>>>>> + u32 ubwc_static)
> >>>>>>>>>>>>> +{
> >>>>>>>>>>>>> + writel_relaxed(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)
> >>>>>>>>>>>>> +{
> >>>>>>>>>>>>> + u32 value = (ubwc_swizzle & 0x1) |
> >>>>>>>>>>>>> + (highest_bank_bit & 0x3) << 4 |
> >>>>>>>>>>>>> + (macrotile_mode & 0x1) << 12;
> >>>>>>>>>>>>> +
> >>>>>>>>>>>>> + if (ubwc_version == UBWC_3_0)
> >>>>>>>>>>>>> + value |= BIT(10);
> >>>>>>>>>>>>> +
> >>>>>>>>>>>>> + if (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)
> >>>>>>>>>>>>> +{
> >>>>>>>>>>>>> + u32 value = (ubwc_swizzle & 0x7) |
> >>>>>>>>>>>>> + (ubwc_static & 0x1) << 3 |
> >>>>>>>>>>>>> + (highest_bank_bit & 0x7) << 4 |
> >>>>>>>>>>>>> + (macrotile_mode & 0x1) << 12;
> >>>>>>>>>>>>> +
> >>>>>>>>>>>>> + writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
> >>>>>>>>>>>>> +
> >>>>>>>>>>>>> + if (ubwc_version == UBWC_3_0) {
> >>>>>>>>>>>>> + writel_relaxed(1, msm_mdss->mmio + UBWC_CTRL_2);
> >>>>>>>>>>>>> + writel_relaxed(0, msm_mdss->mmio +
> >>>>>>>>>>>>> UBWC_PREDICTION_MODE);
> >>>>>>>>>>>>> + } else {
> >>>>>>>>>>>>> + writel_relaxed(2, msm_mdss->mmio + UBWC_CTRL_2);
> >>>>>>>>>>>>> + writel_relaxed(1, msm_mdss->mmio +
> >>>>>>>>>>>>> UBWC_PREDICTION_MODE);
> >>>>>>>>>>>>> + }
> >>>>>>>>>>>>> +}
> >>>>>>>>>>>>> +
> >>>>>>>>>>>>
> >>>>>>>>>>>> Is it possible to unify the above functions by having the
> >>>>>>>>>>>> internal
> >>>>>>>>>>>> ubwc_version checks?
> >>>>>>>>>>>
> >>>>>>>>>>> Note, it's not the ubwc_version, it is the
> >>>>>>>>>>> ubwc_dec_hw_version. And
> >>>>>>>>>>> also different functions take different sets of arguments.
> >>>>>>>>>>>
> >>>>>>>>>>>> It seems like msm_mdss_setup_ubwc_dec_xxx can keep growing.
> >>>>>>>>>>>>
> >>>>>>>>>>>> I have not looked into each bit programming but from the top
> >>>>>>>>>>>> level so
> >>>>>>>>>>>> feel free to correct if wrong but it seems both do write
> >>>>>>>>>>>> UBWC_STATIC
> >>>>>>>>>>>> (different values based on different UBWC versions) and
> >>>>>>>>>>>> write some extra
> >>>>>>>>>>>> registers based on version
> >>>>>>>>>>>
> >>>>>>>>>>> This is what both the current code and the downstream do. See
> >>>>>>>>>>> https://github.com/MiCode/Xiaomi_Kernel_OpenSource/blob/zeus-s-oss/techpack/display-drivers/msm/sde/sde_hw_top.c#L312
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> Thanks for pointing to the downstream method for this,
> >>>>>>>>>>
> >>>>>>>>>> This is exactly what i was also suggesting to do when I mentioned
> >>>>>>>>>> unifying the above functions.
> >>>>>>>>>>
> >>>>>>>>>> So instead of having a separate function for each version why
> >>>>>>>>>> not handle
> >>>>>>>>>> all the versions in the same function like what the link you
> >>>>>>>>>> have shown
> >>>>>>>>>> does.
> >>>>>>>>>
> >>>>>>>>> I wouldn't like that. The downstream uses hw_catalog to pass all
> >>>>>>>>> possible parameters. We do not, so we'd have a whole set of
> >>>>>>>>> artificial
> >>>>>>>>> values.
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> Now that you brought that up, why cannot even upstream dpu start
> >>>>>>>> using
> >>>>>>>> catalog for ubwc settings?
> >>>>>>>
> >>>>>>> Because msm_mdss lives out of disp/dpu1. And using the disp/dpu1 for
> >>>>>>> it would be an inversion of dependencies.
> >>>>>>> I like the fact that msm_mdss is independent of mdp/dpu drivers
> >>>>>>> and I
> >>>>>>> do not want to add such dependency.
> >>>>>>>
> >>>>>>
> >>>>>> Ok, so I think this function itself is placed incorrectly. It
> >>>>>> should not
> >>>>>> be in msm_mdss.c and should in the DPU folder.
> >>>>>>
> >>>>>> This check tells me that this will not be executed for mdp5 devices.
> >>>>>>
> >>>>>> /*
> >>>>>> * 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)
> >>>>>> return 0;
> >>>>>
> >>>>> This condition should be changed to check for the MDP_CLK being
> >>>>> available in the clocks array rather than checking for is_mdp5. I'd
> >>>>> like to phase is_mdp5 away at some point.
> >>>>>
> >>>>>> In that case, what prevents us from moving this to dpu and start
> >>>>>> using
> >>>>>> catalog for this?
> >>>>>
> >>>>> Because there is nothing tying mdss and dpu drivers. For example, is
> >>>>> the msm8998 (3.0.0) the DPU or MDP5 device? MSM8996?
> >>>>> Neither struct msm_mdss nor the MDSS device itself are accessible
> >>>>> through the msm_drv (or dpu_kms).
> >>>>> I think trying to invent such a link would make the code worse.
> >>>>>
> >>>>
> >>>> Right, what I am trying to mention with that check is that means
> >>>> that code does not run today for mdp5 and it still works fine.
> >>>>
> >>>> So why not just move it to DPU first to carry less burden of these
> >>>> extra register settings which are unused today for mdp5 anyway.
> >>>
> >>> As I mentioned, there is no good way. msm_mdss doesn't know about
> >>> DPU. DPU doesn't know about the msm_mdss. Even the msm_drv doesn't
> >>> know about the msm_mdss.
> >>>
> >>> If you can sketch a nice piece of code, could you please demonstrate
> >>> your idea?
> >>>
> >>
> >> No, so I am not suggesting to do it in msm_mdss. Only then you will
> >> need msm_mdss to have knowledge of whether its DPU or MDP5.
> >>
> >> Correct me if wrong. msm_mdss is common to both MDP5 and DPU.
> >>
> >> From the above check its clear that this code does not run for MDP5.
> >>
> >> So I am suggesting move this code completely to dpu_runtime_resume().
> >>
> >> That way you can use catalog there.
> >>
> >> I am questioning why we even need this function to be in msm_mdss. It
> >> can just belong in DPU as its not being used by MDP5 today.
> >
> > The region used by the mdss is not mapped by the DPU (or MDP5). This
> > caused some confusion with DPU code trying to write to the non-mapped or
> > incorrect memory areas before we ended up with Jonathan fixing the code
> > in 544d8b96150d ("drm/msm/dpu: update UBWC config for sm8150 and sm8250").
> >
>
> Thanks so much for the providing this change , it gave me some more context.
>
> I briefly went through this change.
>
> So what happened was dpu_kms->mmio is different from dpu_mdss->mmio in
> terms of offset.
>
> dpu_mdss->mmio is the one which starts from 0 and thats the one we
> should use for the UBWC config.
>
> So even after Jonathan's change we were doing this in dpu_mdss.c
>
> After we removed the dpu_mdss.c layer to start using msm_mdss.c, now
> this function looks somewhat misplaced to me because it was in dpu.
>
> One suggestion I have is, let dpu_kms.c map this region "mdss" by
> getting it from its parent using of_get_parent() and store it as
> "mdss_mmio". Then move this back to dpu_runtime_resume().
>
> Then rework that function to remove these magic numbers and start using
> catalog.
>
> Let me know what you think. Thats the best i can come up with in the
> current design.
I feel this is an overkill for setting just three registers. MDSS
register space is not used for anything else than handling generic
interrupts and setting the UBWC. I'd leave things as is and just
replace cryptic register writes with manageable API.
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Freedreno] [PATCH] drm/msm: less magic numbers in msm_mdss_enable
2022-06-09 6:08 ` Dmitry Baryshkov
@ 2022-06-10 22:23 ` Abhinav Kumar
-1 siblings, 0 replies; 32+ messages in thread
From: Abhinav Kumar @ 2022-06-10 22:23 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Sean Paul, David Airlie, linux-arm-msm, dri-devel,
Bjorn Andersson, Stephen Boyd, freedreno
On 6/8/2022 11:08 PM, Dmitry Baryshkov wrote:
> On Thu, 9 Jun 2022 at 02:37, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 6/8/2022 3:46 PM, Dmitry Baryshkov wrote:
>>> On 09/06/2022 01:42, Abhinav Kumar wrote:
>>>>
>>>>
>>>> On 6/8/2022 3:38 PM, Dmitry Baryshkov wrote:
>>>>> On 09/06/2022 01:35, Abhinav Kumar wrote:
>>>>>>
>>>>>>
>>>>>> On 6/8/2022 3:30 PM, Dmitry Baryshkov wrote:
>>>>>>> On Wed, 8 Jun 2022 at 22:29, Abhinav Kumar
>>>>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 6/2/2022 1:13 PM, Dmitry Baryshkov wrote:
>>>>>>>>> On Thu, 2 Jun 2022 at 21:18, Abhinav Kumar
>>>>>>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 6/1/2022 1:04 PM, Dmitry Baryshkov wrote:
>>>>>>>>>>> On Wed, 1 Jun 2022 at 20:38, Abhinav Kumar
>>>>>>>>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On 6/1/2022 2:46 AM, Dmitry Baryshkov wrote:
>>>>>>>>>>>>> On Wed, 1 Jun 2022 at 01:01, Abhinav Kumar
>>>>>>>>>>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>>>>>>>>>> On 5/31/2022 5:18 AM, Dmitry Baryshkov wrote:
>>>>>>>>>>>>>>> Replace magic register writes in msm_mdss_enable() with
>>>>>>>>>>>>>>> version that
>>>>>>>>>>>>>>> contains less magic and more variable names that can be
>>>>>>>>>>>>>>> traced back to
>>>>>>>>>>>>>>> the dpu_hw_catalog or the downstream dtsi files.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>> drivers/gpu/drm/msm/msm_mdss.c | 79
>>>>>>>>>>>>>>> ++++++++++++++++++++++++++++++----
>>>>>>>>>>>>>>> 1 file changed, 71 insertions(+), 8 deletions(-)
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> diff --git a/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>>>>>>>>>> b/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>>>>>>>>>> index 0454a571adf7..2a48263cd1b5 100644
>>>>>>>>>>>>>>> --- a/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>>>>>>>>>> +++ b/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>>>>>>>>>> @@ -21,6 +21,7 @@
>>>>>>>>>>>>>>> #define HW_REV 0x0
>>>>>>>>>>>>>>> #define HW_INTR_STATUS 0x0010
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> +#define UBWC_DEC_HW_VERSION 0x58
>>>>>>>>>>>>>>> #define UBWC_STATIC 0x144
>>>>>>>>>>>>>>> #define UBWC_CTRL_2 0x150
>>>>>>>>>>>>>>> #define UBWC_PREDICTION_MODE 0x154
>>>>>>>>>>>>>>> @@ -132,9 +133,63 @@ static int
>>>>>>>>>>>>>>> _msm_mdss_irq_domain_add(struct msm_mdss *msm_mdss)
>>>>>>>>>>>>>>> return 0;
>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> +#define UBWC_1_0 0x10000000
>>>>>>>>>>>>>>> +#define UBWC_2_0 0x20000000
>>>>>>>>>>>>>>> +#define UBWC_3_0 0x30000000
>>>>>>>>>>>>>>> +#define UBWC_4_0 0x40000000
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> +static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss
>>>>>>>>>>>>>>> *msm_mdss,
>>>>>>>>>>>>>>> + u32 ubwc_static)
>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>> + writel_relaxed(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)
>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>> + u32 value = (ubwc_swizzle & 0x1) |
>>>>>>>>>>>>>>> + (highest_bank_bit & 0x3) << 4 |
>>>>>>>>>>>>>>> + (macrotile_mode & 0x1) << 12;
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> + if (ubwc_version == UBWC_3_0)
>>>>>>>>>>>>>>> + value |= BIT(10);
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> + if (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)
>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>> + u32 value = (ubwc_swizzle & 0x7) |
>>>>>>>>>>>>>>> + (ubwc_static & 0x1) << 3 |
>>>>>>>>>>>>>>> + (highest_bank_bit & 0x7) << 4 |
>>>>>>>>>>>>>>> + (macrotile_mode & 0x1) << 12;
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> + writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> + if (ubwc_version == UBWC_3_0) {
>>>>>>>>>>>>>>> + writel_relaxed(1, msm_mdss->mmio + UBWC_CTRL_2);
>>>>>>>>>>>>>>> + writel_relaxed(0, msm_mdss->mmio +
>>>>>>>>>>>>>>> UBWC_PREDICTION_MODE);
>>>>>>>>>>>>>>> + } else {
>>>>>>>>>>>>>>> + writel_relaxed(2, msm_mdss->mmio + UBWC_CTRL_2);
>>>>>>>>>>>>>>> + writel_relaxed(1, msm_mdss->mmio +
>>>>>>>>>>>>>>> UBWC_PREDICTION_MODE);
>>>>>>>>>>>>>>> + }
>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Is it possible to unify the above functions by having the
>>>>>>>>>>>>>> internal
>>>>>>>>>>>>>> ubwc_version checks?
>>>>>>>>>>>>>
>>>>>>>>>>>>> Note, it's not the ubwc_version, it is the
>>>>>>>>>>>>> ubwc_dec_hw_version. And
>>>>>>>>>>>>> also different functions take different sets of arguments.
>>>>>>>>>>>>>
>>>>>>>>>>>>>> It seems like msm_mdss_setup_ubwc_dec_xxx can keep growing.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I have not looked into each bit programming but from the top
>>>>>>>>>>>>>> level so
>>>>>>>>>>>>>> feel free to correct if wrong but it seems both do write
>>>>>>>>>>>>>> UBWC_STATIC
>>>>>>>>>>>>>> (different values based on different UBWC versions) and
>>>>>>>>>>>>>> write some extra
>>>>>>>>>>>>>> registers based on version
>>>>>>>>>>>>>
>>>>>>>>>>>>> This is what both the current code and the downstream do. See
>>>>>>>>>>>>> https://github.com/MiCode/Xiaomi_Kernel_OpenSource/blob/zeus-s-oss/techpack/display-drivers/msm/sde/sde_hw_top.c#L312
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks for pointing to the downstream method for this,
>>>>>>>>>>>>
>>>>>>>>>>>> This is exactly what i was also suggesting to do when I mentioned
>>>>>>>>>>>> unifying the above functions.
>>>>>>>>>>>>
>>>>>>>>>>>> So instead of having a separate function for each version why
>>>>>>>>>>>> not handle
>>>>>>>>>>>> all the versions in the same function like what the link you
>>>>>>>>>>>> have shown
>>>>>>>>>>>> does.
>>>>>>>>>>>
>>>>>>>>>>> I wouldn't like that. The downstream uses hw_catalog to pass all
>>>>>>>>>>> possible parameters. We do not, so we'd have a whole set of
>>>>>>>>>>> artificial
>>>>>>>>>>> values.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Now that you brought that up, why cannot even upstream dpu start
>>>>>>>>>> using
>>>>>>>>>> catalog for ubwc settings?
>>>>>>>>>
>>>>>>>>> Because msm_mdss lives out of disp/dpu1. And using the disp/dpu1 for
>>>>>>>>> it would be an inversion of dependencies.
>>>>>>>>> I like the fact that msm_mdss is independent of mdp/dpu drivers
>>>>>>>>> and I
>>>>>>>>> do not want to add such dependency.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Ok, so I think this function itself is placed incorrectly. It
>>>>>>>> should not
>>>>>>>> be in msm_mdss.c and should in the DPU folder.
>>>>>>>>
>>>>>>>> This check tells me that this will not be executed for mdp5 devices.
>>>>>>>>
>>>>>>>> /*
>>>>>>>> * 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)
>>>>>>>> return 0;
>>>>>>>
>>>>>>> This condition should be changed to check for the MDP_CLK being
>>>>>>> available in the clocks array rather than checking for is_mdp5. I'd
>>>>>>> like to phase is_mdp5 away at some point.
>>>>>>>
>>>>>>>> In that case, what prevents us from moving this to dpu and start
>>>>>>>> using
>>>>>>>> catalog for this?
>>>>>>>
>>>>>>> Because there is nothing tying mdss and dpu drivers. For example, is
>>>>>>> the msm8998 (3.0.0) the DPU or MDP5 device? MSM8996?
>>>>>>> Neither struct msm_mdss nor the MDSS device itself are accessible
>>>>>>> through the msm_drv (or dpu_kms).
>>>>>>> I think trying to invent such a link would make the code worse.
>>>>>>>
>>>>>>
>>>>>> Right, what I am trying to mention with that check is that means
>>>>>> that code does not run today for mdp5 and it still works fine.
>>>>>>
>>>>>> So why not just move it to DPU first to carry less burden of these
>>>>>> extra register settings which are unused today for mdp5 anyway.
>>>>>
>>>>> As I mentioned, there is no good way. msm_mdss doesn't know about
>>>>> DPU. DPU doesn't know about the msm_mdss. Even the msm_drv doesn't
>>>>> know about the msm_mdss.
>>>>>
>>>>> If you can sketch a nice piece of code, could you please demonstrate
>>>>> your idea?
>>>>>
>>>>
>>>> No, so I am not suggesting to do it in msm_mdss. Only then you will
>>>> need msm_mdss to have knowledge of whether its DPU or MDP5.
>>>>
>>>> Correct me if wrong. msm_mdss is common to both MDP5 and DPU.
>>>>
>>>> From the above check its clear that this code does not run for MDP5.
>>>>
>>>> So I am suggesting move this code completely to dpu_runtime_resume().
>>>>
>>>> That way you can use catalog there.
>>>>
>>>> I am questioning why we even need this function to be in msm_mdss. It
>>>> can just belong in DPU as its not being used by MDP5 today.
>>>
>>> The region used by the mdss is not mapped by the DPU (or MDP5). This
>>> caused some confusion with DPU code trying to write to the non-mapped or
>>> incorrect memory areas before we ended up with Jonathan fixing the code
>>> in 544d8b96150d ("drm/msm/dpu: update UBWC config for sm8150 and sm8250").
>>>
>>
>> Thanks so much for the providing this change , it gave me some more context.
>>
>> I briefly went through this change.
>>
>> So what happened was dpu_kms->mmio is different from dpu_mdss->mmio in
>> terms of offset.
>>
>> dpu_mdss->mmio is the one which starts from 0 and thats the one we
>> should use for the UBWC config.
>>
>> So even after Jonathan's change we were doing this in dpu_mdss.c
>>
>> After we removed the dpu_mdss.c layer to start using msm_mdss.c, now
>> this function looks somewhat misplaced to me because it was in dpu.
>>
>> One suggestion I have is, let dpu_kms.c map this region "mdss" by
>> getting it from its parent using of_get_parent() and store it as
>> "mdss_mmio". Then move this back to dpu_runtime_resume().
>>
>> Then rework that function to remove these magic numbers and start using
>> catalog.
>>
>> Let me know what you think. Thats the best i can come up with in the
>> current design.
>
> I feel this is an overkill for setting just three registers. MDSS
> register space is not used for anything else than handling generic
> interrupts and setting the UBWC. I'd leave things as is and just
> replace cryptic register writes with manageable API.
>
Alright, as we discussed during the meeting, both of us are not really
too happy with how this API turned out and also because we cannot use
catalog anymore for these registers after the dpu_mdss ---> msm_mdss
separation. If the number of UBWC versions keeps increasing like this,
we will have to re-visit this of moving this back to DPU with the
solution I have suggested above and start using catalog.
I am not going to hold this patch back but we will re-visit the below
comment if we get more versions of UBWC.
+ *
+ * Decoder version can be read from the UBWC_DEC_HW_VERSION reg,
+ * UBWC_n comes from hw_catalog.
+ * Unforunately this driver can not access hw catalog.
*/
As I commented previously, please either drop the below print from here
OR push another change to remove it from
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c#L1096
+ dev_info(msm_mdss->dev, "HW_REV: 0x%x\n", hw_rev);
+ * Unforunately this driver can not access hw catalog.
With the above print message concern addressed,
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Freedreno] [PATCH] drm/msm: less magic numbers in msm_mdss_enable
@ 2022-06-10 22:23 ` Abhinav Kumar
0 siblings, 0 replies; 32+ messages in thread
From: Abhinav Kumar @ 2022-06-10 22:23 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: freedreno, David Airlie, linux-arm-msm, dri-devel, Stephen Boyd,
Bjorn Andersson, Sean Paul
On 6/8/2022 11:08 PM, Dmitry Baryshkov wrote:
> On Thu, 9 Jun 2022 at 02:37, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 6/8/2022 3:46 PM, Dmitry Baryshkov wrote:
>>> On 09/06/2022 01:42, Abhinav Kumar wrote:
>>>>
>>>>
>>>> On 6/8/2022 3:38 PM, Dmitry Baryshkov wrote:
>>>>> On 09/06/2022 01:35, Abhinav Kumar wrote:
>>>>>>
>>>>>>
>>>>>> On 6/8/2022 3:30 PM, Dmitry Baryshkov wrote:
>>>>>>> On Wed, 8 Jun 2022 at 22:29, Abhinav Kumar
>>>>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 6/2/2022 1:13 PM, Dmitry Baryshkov wrote:
>>>>>>>>> On Thu, 2 Jun 2022 at 21:18, Abhinav Kumar
>>>>>>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 6/1/2022 1:04 PM, Dmitry Baryshkov wrote:
>>>>>>>>>>> On Wed, 1 Jun 2022 at 20:38, Abhinav Kumar
>>>>>>>>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On 6/1/2022 2:46 AM, Dmitry Baryshkov wrote:
>>>>>>>>>>>>> On Wed, 1 Jun 2022 at 01:01, Abhinav Kumar
>>>>>>>>>>>>> <quic_abhinavk@quicinc.com> wrote:
>>>>>>>>>>>>>> On 5/31/2022 5:18 AM, Dmitry Baryshkov wrote:
>>>>>>>>>>>>>>> Replace magic register writes in msm_mdss_enable() with
>>>>>>>>>>>>>>> version that
>>>>>>>>>>>>>>> contains less magic and more variable names that can be
>>>>>>>>>>>>>>> traced back to
>>>>>>>>>>>>>>> the dpu_hw_catalog or the downstream dtsi files.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>> drivers/gpu/drm/msm/msm_mdss.c | 79
>>>>>>>>>>>>>>> ++++++++++++++++++++++++++++++----
>>>>>>>>>>>>>>> 1 file changed, 71 insertions(+), 8 deletions(-)
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> diff --git a/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>>>>>>>>>> b/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>>>>>>>>>> index 0454a571adf7..2a48263cd1b5 100644
>>>>>>>>>>>>>>> --- a/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>>>>>>>>>> +++ b/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>>>>>>>>>> @@ -21,6 +21,7 @@
>>>>>>>>>>>>>>> #define HW_REV 0x0
>>>>>>>>>>>>>>> #define HW_INTR_STATUS 0x0010
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> +#define UBWC_DEC_HW_VERSION 0x58
>>>>>>>>>>>>>>> #define UBWC_STATIC 0x144
>>>>>>>>>>>>>>> #define UBWC_CTRL_2 0x150
>>>>>>>>>>>>>>> #define UBWC_PREDICTION_MODE 0x154
>>>>>>>>>>>>>>> @@ -132,9 +133,63 @@ static int
>>>>>>>>>>>>>>> _msm_mdss_irq_domain_add(struct msm_mdss *msm_mdss)
>>>>>>>>>>>>>>> return 0;
>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> +#define UBWC_1_0 0x10000000
>>>>>>>>>>>>>>> +#define UBWC_2_0 0x20000000
>>>>>>>>>>>>>>> +#define UBWC_3_0 0x30000000
>>>>>>>>>>>>>>> +#define UBWC_4_0 0x40000000
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> +static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss
>>>>>>>>>>>>>>> *msm_mdss,
>>>>>>>>>>>>>>> + u32 ubwc_static)
>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>> + writel_relaxed(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)
>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>> + u32 value = (ubwc_swizzle & 0x1) |
>>>>>>>>>>>>>>> + (highest_bank_bit & 0x3) << 4 |
>>>>>>>>>>>>>>> + (macrotile_mode & 0x1) << 12;
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> + if (ubwc_version == UBWC_3_0)
>>>>>>>>>>>>>>> + value |= BIT(10);
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> + if (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)
>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>> + u32 value = (ubwc_swizzle & 0x7) |
>>>>>>>>>>>>>>> + (ubwc_static & 0x1) << 3 |
>>>>>>>>>>>>>>> + (highest_bank_bit & 0x7) << 4 |
>>>>>>>>>>>>>>> + (macrotile_mode & 0x1) << 12;
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> + writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> + if (ubwc_version == UBWC_3_0) {
>>>>>>>>>>>>>>> + writel_relaxed(1, msm_mdss->mmio + UBWC_CTRL_2);
>>>>>>>>>>>>>>> + writel_relaxed(0, msm_mdss->mmio +
>>>>>>>>>>>>>>> UBWC_PREDICTION_MODE);
>>>>>>>>>>>>>>> + } else {
>>>>>>>>>>>>>>> + writel_relaxed(2, msm_mdss->mmio + UBWC_CTRL_2);
>>>>>>>>>>>>>>> + writel_relaxed(1, msm_mdss->mmio +
>>>>>>>>>>>>>>> UBWC_PREDICTION_MODE);
>>>>>>>>>>>>>>> + }
>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Is it possible to unify the above functions by having the
>>>>>>>>>>>>>> internal
>>>>>>>>>>>>>> ubwc_version checks?
>>>>>>>>>>>>>
>>>>>>>>>>>>> Note, it's not the ubwc_version, it is the
>>>>>>>>>>>>> ubwc_dec_hw_version. And
>>>>>>>>>>>>> also different functions take different sets of arguments.
>>>>>>>>>>>>>
>>>>>>>>>>>>>> It seems like msm_mdss_setup_ubwc_dec_xxx can keep growing.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I have not looked into each bit programming but from the top
>>>>>>>>>>>>>> level so
>>>>>>>>>>>>>> feel free to correct if wrong but it seems both do write
>>>>>>>>>>>>>> UBWC_STATIC
>>>>>>>>>>>>>> (different values based on different UBWC versions) and
>>>>>>>>>>>>>> write some extra
>>>>>>>>>>>>>> registers based on version
>>>>>>>>>>>>>
>>>>>>>>>>>>> This is what both the current code and the downstream do. See
>>>>>>>>>>>>> https://github.com/MiCode/Xiaomi_Kernel_OpenSource/blob/zeus-s-oss/techpack/display-drivers/msm/sde/sde_hw_top.c#L312
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks for pointing to the downstream method for this,
>>>>>>>>>>>>
>>>>>>>>>>>> This is exactly what i was also suggesting to do when I mentioned
>>>>>>>>>>>> unifying the above functions.
>>>>>>>>>>>>
>>>>>>>>>>>> So instead of having a separate function for each version why
>>>>>>>>>>>> not handle
>>>>>>>>>>>> all the versions in the same function like what the link you
>>>>>>>>>>>> have shown
>>>>>>>>>>>> does.
>>>>>>>>>>>
>>>>>>>>>>> I wouldn't like that. The downstream uses hw_catalog to pass all
>>>>>>>>>>> possible parameters. We do not, so we'd have a whole set of
>>>>>>>>>>> artificial
>>>>>>>>>>> values.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Now that you brought that up, why cannot even upstream dpu start
>>>>>>>>>> using
>>>>>>>>>> catalog for ubwc settings?
>>>>>>>>>
>>>>>>>>> Because msm_mdss lives out of disp/dpu1. And using the disp/dpu1 for
>>>>>>>>> it would be an inversion of dependencies.
>>>>>>>>> I like the fact that msm_mdss is independent of mdp/dpu drivers
>>>>>>>>> and I
>>>>>>>>> do not want to add such dependency.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Ok, so I think this function itself is placed incorrectly. It
>>>>>>>> should not
>>>>>>>> be in msm_mdss.c and should in the DPU folder.
>>>>>>>>
>>>>>>>> This check tells me that this will not be executed for mdp5 devices.
>>>>>>>>
>>>>>>>> /*
>>>>>>>> * 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)
>>>>>>>> return 0;
>>>>>>>
>>>>>>> This condition should be changed to check for the MDP_CLK being
>>>>>>> available in the clocks array rather than checking for is_mdp5. I'd
>>>>>>> like to phase is_mdp5 away at some point.
>>>>>>>
>>>>>>>> In that case, what prevents us from moving this to dpu and start
>>>>>>>> using
>>>>>>>> catalog for this?
>>>>>>>
>>>>>>> Because there is nothing tying mdss and dpu drivers. For example, is
>>>>>>> the msm8998 (3.0.0) the DPU or MDP5 device? MSM8996?
>>>>>>> Neither struct msm_mdss nor the MDSS device itself are accessible
>>>>>>> through the msm_drv (or dpu_kms).
>>>>>>> I think trying to invent such a link would make the code worse.
>>>>>>>
>>>>>>
>>>>>> Right, what I am trying to mention with that check is that means
>>>>>> that code does not run today for mdp5 and it still works fine.
>>>>>>
>>>>>> So why not just move it to DPU first to carry less burden of these
>>>>>> extra register settings which are unused today for mdp5 anyway.
>>>>>
>>>>> As I mentioned, there is no good way. msm_mdss doesn't know about
>>>>> DPU. DPU doesn't know about the msm_mdss. Even the msm_drv doesn't
>>>>> know about the msm_mdss.
>>>>>
>>>>> If you can sketch a nice piece of code, could you please demonstrate
>>>>> your idea?
>>>>>
>>>>
>>>> No, so I am not suggesting to do it in msm_mdss. Only then you will
>>>> need msm_mdss to have knowledge of whether its DPU or MDP5.
>>>>
>>>> Correct me if wrong. msm_mdss is common to both MDP5 and DPU.
>>>>
>>>> From the above check its clear that this code does not run for MDP5.
>>>>
>>>> So I am suggesting move this code completely to dpu_runtime_resume().
>>>>
>>>> That way you can use catalog there.
>>>>
>>>> I am questioning why we even need this function to be in msm_mdss. It
>>>> can just belong in DPU as its not being used by MDP5 today.
>>>
>>> The region used by the mdss is not mapped by the DPU (or MDP5). This
>>> caused some confusion with DPU code trying to write to the non-mapped or
>>> incorrect memory areas before we ended up with Jonathan fixing the code
>>> in 544d8b96150d ("drm/msm/dpu: update UBWC config for sm8150 and sm8250").
>>>
>>
>> Thanks so much for the providing this change , it gave me some more context.
>>
>> I briefly went through this change.
>>
>> So what happened was dpu_kms->mmio is different from dpu_mdss->mmio in
>> terms of offset.
>>
>> dpu_mdss->mmio is the one which starts from 0 and thats the one we
>> should use for the UBWC config.
>>
>> So even after Jonathan's change we were doing this in dpu_mdss.c
>>
>> After we removed the dpu_mdss.c layer to start using msm_mdss.c, now
>> this function looks somewhat misplaced to me because it was in dpu.
>>
>> One suggestion I have is, let dpu_kms.c map this region "mdss" by
>> getting it from its parent using of_get_parent() and store it as
>> "mdss_mmio". Then move this back to dpu_runtime_resume().
>>
>> Then rework that function to remove these magic numbers and start using
>> catalog.
>>
>> Let me know what you think. Thats the best i can come up with in the
>> current design.
>
> I feel this is an overkill for setting just three registers. MDSS
> register space is not used for anything else than handling generic
> interrupts and setting the UBWC. I'd leave things as is and just
> replace cryptic register writes with manageable API.
>
Alright, as we discussed during the meeting, both of us are not really
too happy with how this API turned out and also because we cannot use
catalog anymore for these registers after the dpu_mdss ---> msm_mdss
separation. If the number of UBWC versions keeps increasing like this,
we will have to re-visit this of moving this back to DPU with the
solution I have suggested above and start using catalog.
I am not going to hold this patch back but we will re-visit the below
comment if we get more versions of UBWC.
+ *
+ * Decoder version can be read from the UBWC_DEC_HW_VERSION reg,
+ * UBWC_n comes from hw_catalog.
+ * Unforunately this driver can not access hw catalog.
*/
As I commented previously, please either drop the below print from here
OR push another change to remove it from
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c#L1096
+ dev_info(msm_mdss->dev, "HW_REV: 0x%x\n", hw_rev);
+ * Unforunately this driver can not access hw catalog.
With the above print message concern addressed,
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
^ permalink raw reply [flat|nested] 32+ messages in thread
end of thread, other threads:[~2022-06-10 22:23 UTC | newest]
Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-31 12:18 [PATCH] drm/msm: less magic numbers in msm_mdss_enable Dmitry Baryshkov
2022-05-31 12:18 ` Dmitry Baryshkov
2022-05-31 22:00 ` Abhinav Kumar
2022-05-31 22:00 ` Abhinav Kumar
2022-06-01 9:46 ` Dmitry Baryshkov
2022-06-01 9:46 ` Dmitry Baryshkov
2022-06-01 17:37 ` [Freedreno] " Abhinav Kumar
2022-06-01 17:37 ` Abhinav Kumar
2022-06-01 20:04 ` Dmitry Baryshkov
2022-06-01 20:04 ` Dmitry Baryshkov
2022-06-02 18:18 ` Abhinav Kumar
2022-06-02 18:18 ` Abhinav Kumar
2022-06-02 20:13 ` Dmitry Baryshkov
2022-06-02 20:13 ` Dmitry Baryshkov
2022-06-08 19:29 ` Abhinav Kumar
2022-06-08 19:29 ` Abhinav Kumar
2022-06-08 22:30 ` Dmitry Baryshkov
2022-06-08 22:30 ` Dmitry Baryshkov
2022-06-08 22:35 ` Abhinav Kumar
2022-06-08 22:35 ` Abhinav Kumar
2022-06-08 22:38 ` Dmitry Baryshkov
2022-06-08 22:38 ` Dmitry Baryshkov
2022-06-08 22:42 ` Abhinav Kumar
2022-06-08 22:42 ` Abhinav Kumar
2022-06-08 22:46 ` Dmitry Baryshkov
2022-06-08 22:46 ` Dmitry Baryshkov
2022-06-08 23:37 ` Abhinav Kumar
2022-06-08 23:37 ` Abhinav Kumar
2022-06-09 6:08 ` Dmitry Baryshkov
2022-06-09 6:08 ` Dmitry Baryshkov
2022-06-10 22:23 ` Abhinav Kumar
2022-06-10 22:23 ` Abhinav Kumar
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.