Re: [Freedreno] [PATCH] drm/msm: less magic numbers in msm_mdss_enable
On 6/8/2022 11:08 PM, Dmitry Baryshkov wrote:
On Thu, 9 Jun 2022 at 02:37, Abhinav Kumar 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
wrote:
On 6/2/2022 1:13 PM, Dmitry Baryshkov wrote:
On Thu, 2 Jun 2022 at 21:18, Abhinav Kumar
wrote:
On 6/1/2022 1:04 PM, Dmitry Baryshkov wrote:
On Wed, 1 Jun 2022 at 20:38, Abhinav Kumar
wrote:
On 6/1/2022 2:46 AM, Dmitry Baryshkov wrote:
On Wed, 1 Jun 2022 at 01:01, Abhinav Kumar
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
---
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_MODE0x154
@@ -132,9 +133,63 @@ static int
_msm_mdss_irq_domain_add(struct msm_mdss *msm_mdss)
return 0;
}
+#define UBWC_1_0 0x1000
+#define UBWC_2_0 0x2000
+#define UBWC_3_0 0x3000
+#define UBWC_4_0 0x4000
+
+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
Re: [Freedreno] [PATCH] drm/msm: less magic numbers in msm_mdss_enable
On Thu, 9 Jun 2022 at 02:37, Abhinav Kumar 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
> > wrote:
> >>
> >>
> >>
> >> On 6/2/2022 1:13 PM, Dmitry Baryshkov wrote:
> >>> On Thu, 2 Jun 2022 at 21:18, Abhinav Kumar
> >>> wrote:
>
>
>
> On 6/1/2022 1:04 PM, Dmitry Baryshkov wrote:
> > On Wed, 1 Jun 2022 at 20:38, Abhinav Kumar
> > wrote:
> >>
> >>
> >>
> >> On 6/1/2022 2:46 AM, Dmitry Baryshkov wrote:
> >>> On Wed, 1 Jun 2022 at 01:01, Abhinav Kumar
> >>> 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
> > ---
> > 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_MODE0x154
> > @@ -132,9 +133,63 @@ static int
> > _msm_mdss_irq_domain_add(struct msm_mdss *msm_mdss)
> > return 0;
> > }
> >
> > +#define UBWC_1_0 0x1000
> > +#define UBWC_2_0 0x2000
> > +#define UBWC_3_0 0x3000
> > +#define UBWC_4_0 0x4000
> > +
> > +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 +
> >
Re: [Freedreno] [PATCH] drm/msm: less magic numbers in msm_mdss_enable
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
wrote:
On 6/2/2022 1:13 PM, Dmitry Baryshkov wrote:
On Thu, 2 Jun 2022 at 21:18, Abhinav Kumar
wrote:
On 6/1/2022 1:04 PM, Dmitry Baryshkov wrote:
On Wed, 1 Jun 2022 at 20:38, Abhinav Kumar
wrote:
On 6/1/2022 2:46 AM, Dmitry Baryshkov wrote:
On Wed, 1 Jun 2022 at 01:01, Abhinav Kumar
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
---
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 0x1000
+#define UBWC_2_0 0x2000
+#define UBWC_3_0 0x3000
+#define UBWC_4_0 0x4000
+
+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
Re: [Freedreno] [PATCH] drm/msm: less magic numbers in msm_mdss_enable
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
wrote:
On 6/2/2022 1:13 PM, Dmitry Baryshkov wrote:
On Thu, 2 Jun 2022 at 21:18, Abhinav Kumar
wrote:
On 6/1/2022 1:04 PM, Dmitry Baryshkov wrote:
On Wed, 1 Jun 2022 at 20:38, Abhinav Kumar
wrote:
On 6/1/2022 2:46 AM, Dmitry Baryshkov wrote:
On Wed, 1 Jun 2022 at 01:01, Abhinav Kumar
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
---
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 0x1000
+#define UBWC_2_0 0x2000
+#define UBWC_3_0 0x3000
+#define UBWC_4_0 0x4000
+
+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,
Re: [Freedreno] [PATCH] drm/msm: less magic numbers in msm_mdss_enable
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
wrote:
On 6/2/2022 1:13 PM, Dmitry Baryshkov wrote:
On Thu, 2 Jun 2022 at 21:18, Abhinav Kumar
wrote:
On 6/1/2022 1:04 PM, Dmitry Baryshkov wrote:
On Wed, 1 Jun 2022 at 20:38, Abhinav Kumar
wrote:
On 6/1/2022 2:46 AM, Dmitry Baryshkov wrote:
On Wed, 1 Jun 2022 at 01:01, Abhinav Kumar
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
---
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 0x1000
+#define UBWC_2_0 0x2000
+#define UBWC_3_0 0x3000
+#define UBWC_4_0 0x4000
+
+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
Re: [Freedreno] [PATCH] drm/msm: less magic numbers in msm_mdss_enable
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
wrote:
On 6/2/2022 1:13 PM, Dmitry Baryshkov wrote:
On Thu, 2 Jun 2022 at 21:18, Abhinav Kumar
wrote:
On 6/1/2022 1:04 PM, Dmitry Baryshkov wrote:
On Wed, 1 Jun 2022 at 20:38, Abhinav Kumar
wrote:
On 6/1/2022 2:46 AM, Dmitry Baryshkov wrote:
On Wed, 1 Jun 2022 at 01:01, Abhinav Kumar
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
---
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 0x1000
+#define UBWC_2_0 0x2000
+#define UBWC_3_0 0x3000
+#define UBWC_4_0 0x4000
+
+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
Re: [Freedreno] [PATCH] drm/msm: less magic numbers in msm_mdss_enable
On 6/8/2022 3:30 PM, Dmitry Baryshkov wrote:
On Wed, 8 Jun 2022 at 22:29, Abhinav Kumar wrote:
On 6/2/2022 1:13 PM, Dmitry Baryshkov wrote:
On Thu, 2 Jun 2022 at 21:18, Abhinav Kumar wrote:
On 6/1/2022 1:04 PM, Dmitry Baryshkov wrote:
On Wed, 1 Jun 2022 at 20:38, Abhinav Kumar wrote:
On 6/1/2022 2:46 AM, Dmitry Baryshkov wrote:
On Wed, 1 Jun 2022 at 01:01, Abhinav Kumar 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
---
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_MODE0x154
@@ -132,9 +133,63 @@ static int _msm_mdss_irq_domain_add(struct msm_mdss
*msm_mdss)
return 0;
}
+#define UBWC_1_0 0x1000
+#define UBWC_2_0 0x2000
+#define UBWC_3_0 0x3000
+#define UBWC_4_0 0x4000
+
+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
Re: [Freedreno] [PATCH] drm/msm: less magic numbers in msm_mdss_enable
On Wed, 8 Jun 2022 at 22:29, Abhinav Kumar wrote:
>
>
>
> On 6/2/2022 1:13 PM, Dmitry Baryshkov wrote:
> > On Thu, 2 Jun 2022 at 21:18, Abhinav Kumar
> > wrote:
> >>
> >>
> >>
> >> On 6/1/2022 1:04 PM, Dmitry Baryshkov wrote:
> >>> On Wed, 1 Jun 2022 at 20:38, Abhinav Kumar
> >>> wrote:
>
>
>
> On 6/1/2022 2:46 AM, Dmitry Baryshkov wrote:
> > On Wed, 1 Jun 2022 at 01:01, Abhinav Kumar
> > 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
> >>> ---
> >>> 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_MODE0x154
> >>> @@ -132,9 +133,63 @@ static int _msm_mdss_irq_domain_add(struct
> >>> msm_mdss *msm_mdss)
> >>> return 0;
> >>> }
> >>>
> >>> +#define UBWC_1_0 0x1000
> >>> +#define UBWC_2_0 0x2000
> >>> +#define UBWC_3_0 0x3000
> >>> +#define UBWC_4_0 0x4000
> >>> +
> >>> +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
> >
Re: [Freedreno] [PATCH] drm/msm: less magic numbers in msm_mdss_enable
On 6/2/2022 1:13 PM, Dmitry Baryshkov wrote:
On Thu, 2 Jun 2022 at 21:18, Abhinav Kumar wrote:
On 6/1/2022 1:04 PM, Dmitry Baryshkov wrote:
On Wed, 1 Jun 2022 at 20:38, Abhinav Kumar wrote:
On 6/1/2022 2:46 AM, Dmitry Baryshkov wrote:
On Wed, 1 Jun 2022 at 01:01, Abhinav Kumar 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
---
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_MODE0x154
@@ -132,9 +133,63 @@ static int _msm_mdss_irq_domain_add(struct msm_mdss
*msm_mdss)
return 0;
}
+#define UBWC_1_0 0x1000
+#define UBWC_2_0 0x2000
+#define UBWC_3_0 0x3000
+#define UBWC_4_0 0x4000
+
+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
Re: [Freedreno] [PATCH] drm/msm: less magic numbers in msm_mdss_enable
On Thu, 2 Jun 2022 at 21:18, Abhinav Kumar wrote:
>
>
>
> On 6/1/2022 1:04 PM, Dmitry Baryshkov wrote:
> > On Wed, 1 Jun 2022 at 20:38, Abhinav Kumar
> > wrote:
> >>
> >>
> >>
> >> On 6/1/2022 2:46 AM, Dmitry Baryshkov wrote:
> >>> On Wed, 1 Jun 2022 at 01:01, Abhinav Kumar
> >>> 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
> > ---
> > 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_MODE0x154
> > @@ -132,9 +133,63 @@ static int _msm_mdss_irq_domain_add(struct
> > msm_mdss *msm_mdss)
> > return 0;
> > }
> >
> > +#define UBWC_1_0 0x1000
> > +#define UBWC_2_0 0x2000
> > +#define UBWC_3_0 0x3000
> > +#define UBWC_4_0 0x4000
> > +
> > +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
Re: [Freedreno] [PATCH] drm/msm: less magic numbers in msm_mdss_enable
On 6/1/2022 1:04 PM, Dmitry Baryshkov wrote:
On Wed, 1 Jun 2022 at 20:38, Abhinav Kumar wrote:
On 6/1/2022 2:46 AM, Dmitry Baryshkov wrote:
On Wed, 1 Jun 2022 at 01:01, Abhinav Kumar 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
---
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_MODE0x154
@@ -132,9 +133,63 @@ static int _msm_mdss_irq_domain_add(struct msm_mdss
*msm_mdss)
return 0;
}
+#define UBWC_1_0 0x1000
+#define UBWC_2_0 0x2000
+#define UBWC_3_0 0x3000
+#define UBWC_4_0 0x4000
+
+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
Re: [Freedreno] [PATCH] drm/msm: less magic numbers in msm_mdss_enable
On Wed, 1 Jun 2022 at 20:38, Abhinav Kumar wrote:
>
>
>
> On 6/1/2022 2:46 AM, Dmitry Baryshkov wrote:
> > On Wed, 1 Jun 2022 at 01:01, Abhinav Kumar
> > 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
> >>> ---
> >>>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_MODE0x154
> >>> @@ -132,9 +133,63 @@ static int _msm_mdss_irq_domain_add(struct msm_mdss
> >>> *msm_mdss)
> >>>return 0;
> >>>}
> >>>
> >>> +#define UBWC_1_0 0x1000
> >>> +#define UBWC_2_0 0x2000
> >>> +#define UBWC_3_0 0x3000
> >>> +#define UBWC_4_0 0x4000
> >>> +
> >>> +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,
>
Re: [Freedreno] [PATCH] drm/msm: less magic numbers in msm_mdss_enable
On 6/1/2022 2:46 AM, Dmitry Baryshkov wrote:
On Wed, 1 Jun 2022 at 01:01, Abhinav Kumar 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
---
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_MODE0x154
@@ -132,9 +133,63 @@ static int _msm_mdss_irq_domain_add(struct msm_mdss
*msm_mdss)
return 0;
}
+#define UBWC_1_0 0x1000
+#define UBWC_2_0 0x2000
+#define UBWC_3_0 0x3000
+#define UBWC_4_0 0x4000
+
+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]
Re: [Freedreno] [PATCH] drm/msm: less magic numbers in msm_mdss_enable
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
---
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_REV0x0
#define HW_INTR_STATUS0x0010
+#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 0x1000
+#define UBWC_2_0 0x2000
+#define UBWC_3_0 0x3000
+#define UBWC_4_0 0x4000
+
+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,
[Freedreno] [PATCH] drm/msm: less magic numbers in msm_mdss_enable
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
---
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_VERSION0x58
#define UBWC_STATIC0x144
#define UBWC_CTRL_20x150
#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 0x1000
+#define UBWC_2_0 0x2000
+#define UBWC_3_0 0x3000
+#define UBWC_4_0 0x4000
+
+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);
