[PATCH] drm/arm/mali-dp: Add display QoS interface configuration

[PATCH] drm/arm/mali-dp: Add display QoS interface configuration

[Wen He]

Configure the display Quality of service (QoS) levels to high priority
if the level is defined as high as in DTS. The ARQOS for DP500 is driven
from the "RQOS" register, needed to program the RQOS register value < 7
for the 4k resolution flicker to disappear on the LS1028A platform.

Signed-off-by: Wen He <wen.he_1@nxp.com>
---
change in v2:
        - add new implementation for 4k flicker issue on the LS1028A

 drivers/gpu/drm/arm/malidp_drv.c  |  5 +++++
 drivers/gpu/drm/arm/malidp_hw.c   | 13 +++++++++++++
 drivers/gpu/drm/arm/malidp_hw.h   |  3 +++
 drivers/gpu/drm/arm/malidp_regs.h | 12 ++++++++++++
 4 files changed, 33 insertions(+)

diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
index f25ec4382277..d2b2cf52ac87 100644
--- a/drivers/gpu/drm/arm/malidp_drv.c
+++ b/drivers/gpu/drm/arm/malidp_drv.c
@@ -818,6 +818,11 @@ static int malidp_bind(struct device *dev)
 
 	malidp->core_id = version;
 
+	hwdev->arqos_high_level = false;
+
+	hwdev->arqos_high_level = of_property_read_bool(dev->of_node,
+					"arm,malidp-arqos-high-level");
+
 	/* set the number of lines used for output of RGB data */
 	ret = of_property_read_u8_array(dev->of_node,
 					"arm,malidp-output-port-lines",
diff --git a/drivers/gpu/drm/arm/malidp_hw.c b/drivers/gpu/drm/arm/malidp_hw.c
index 50af399d7f6f..eaa1658cd86b 100644
--- a/drivers/gpu/drm/arm/malidp_hw.c
+++ b/drivers/gpu/drm/arm/malidp_hw.c
@@ -374,6 +374,19 @@ static void malidp500_modeset(struct malidp_hw_device *hwdev, struct videomode *
 		malidp_hw_setbits(hwdev, MALIDP_DISP_FUNC_ILACED, MALIDP_DE_DISPLAY_FUNC);
 	else
 		malidp_hw_clearbits(hwdev, MALIDP_DISP_FUNC_ILACED, MALIDP_DE_DISPLAY_FUNC);
+
+	/*
+	 *  Program the RQoS register to increasing QoS levels for
+	 *  the 4k resolution flicker to disappear on the LS1028A.
+	 */
+	if (hwdev->arqos_high_level) {
+		val = RED_ARQOS_VALUE | GREEN_ARQOS_VALUE;
+
+		if (mode->pixelclock == 594000000)
+			malidp_hw_setbits(hwdev, val, MALIDP500_RQOS_QUALITY);
+		else
+			malidp_hw_clearbits(hwdev, val, MALIDP500_RQOS_QUALITY);
+	}
 }
 
 int malidp_format_get_bpp(u32 fmt)
diff --git a/drivers/gpu/drm/arm/malidp_hw.h b/drivers/gpu/drm/arm/malidp_hw.h
index 968a65eed371..b8baba60508a 100644
--- a/drivers/gpu/drm/arm/malidp_hw.h
+++ b/drivers/gpu/drm/arm/malidp_hw.h
@@ -251,6 +251,9 @@ struct malidp_hw_device {
 
 	/* size of memory used for rotating layers, up to two banks available */
 	u32 rotation_memory[2];
+
+	/* priority level of RQOS register used for driven the ARQOS signal */
+	bool arqos_high_level;
 };
 
 static inline u32 malidp_hw_read(struct malidp_hw_device *hwdev, u32 reg)
diff --git a/drivers/gpu/drm/arm/malidp_regs.h b/drivers/gpu/drm/arm/malidp_regs.h
index 993031542fa1..08842142b3b2 100644
--- a/drivers/gpu/drm/arm/malidp_regs.h
+++ b/drivers/gpu/drm/arm/malidp_regs.h
@@ -210,6 +210,18 @@
 #define MALIDP500_CONFIG_VALID		0x00f00
 #define MALIDP500_CONFIG_ID		0x00fd4
 
+/*
+ * The quality of service (QoS) register on the DP500. RQOS register values
+ * are driven by the ARQOS signal, using AXI transacations, dependent on the
+ * FIFO input level.
+ * The RQOS register can also set QoS levels for:
+ *    - RED_ARQOS   @ A 4-bit signal value for close to underflow conditions
+ *    - GREEN_ARQOS @ A 4-bit signal value for normal conditions
+ */
+#define MALIDP500_RQOS_QUALITY          0x00500
+#define RED_ARQOS_VALUE                 (0xd << 12)
+#define GREEN_ARQOS_VALUE               (0xd << 28)
+
 /* register offsets and bits specific to DP550/DP650 */
 #define MALIDP550_ADDR_SPACE_SIZE	0x10000
 #define MALIDP550_DE_CONTROL		0x00010
-- 
2.17.1

Re: [PATCH] drm/arm/mali-dp: Add display QoS interface configuration

[Liviu Dudau]

Hi Wen,

On Wed, Jul 17, 2019 at 05:23:53PM +0800, Wen He wrote:
> Configure the display Quality of service (QoS) levels to high priority
> if the level is defined as high as in DTS. The ARQOS for DP500 is driven
> from the "RQOS" register, needed to program the RQOS register value < 7
> for the 4k resolution flicker to disappear on the LS1028A platform.

Thanks for taking time to come up with a more generic patch for your issue!

I have a question: what happens if you program the MALIDP500_RQOS_QUALITY
register to 0xd000d000 for all pixelclocks?

Also, some suggestions further down:

> 
> Signed-off-by: Wen He <wen.he_1@nxp.com>
> ---
> change in v2:
>         - add new implementation for 4k flicker issue on the LS1028A
> 
>  drivers/gpu/drm/arm/malidp_drv.c  |  5 +++++
>  drivers/gpu/drm/arm/malidp_hw.c   | 13 +++++++++++++
>  drivers/gpu/drm/arm/malidp_hw.h   |  3 +++
>  drivers/gpu/drm/arm/malidp_regs.h | 12 ++++++++++++
>  4 files changed, 33 insertions(+)
> 
> diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
> index f25ec4382277..d2b2cf52ac87 100644
> --- a/drivers/gpu/drm/arm/malidp_drv.c
> +++ b/drivers/gpu/drm/arm/malidp_drv.c
> @@ -818,6 +818,11 @@ static int malidp_bind(struct device *dev)
>  
>  	malidp->core_id = version;
>  
> +	hwdev->arqos_high_level = false;

This is not needed.

> +
> +	hwdev->arqos_high_level = of_property_read_bool(dev->of_node,
> +					"arm,malidp-arqos-high-level");

I think it would be better to have this property as a u32 value, rather than a
boolean, and put the value that we want to program MALIDP_RQOS_QUALITY with in
there.

Also, you need to add the documentation for this optional property in
Documentation/devicetree/bindings/display/arm,malidp.txt.

> +
>  	/* set the number of lines used for output of RGB data */
>  	ret = of_property_read_u8_array(dev->of_node,
>  					"arm,malidp-output-port-lines",
> diff --git a/drivers/gpu/drm/arm/malidp_hw.c b/drivers/gpu/drm/arm/malidp_hw.c
> index 50af399d7f6f..eaa1658cd86b 100644
> --- a/drivers/gpu/drm/arm/malidp_hw.c
> +++ b/drivers/gpu/drm/arm/malidp_hw.c
> @@ -374,6 +374,19 @@ static void malidp500_modeset(struct malidp_hw_device *hwdev, struct videomode *
>  		malidp_hw_setbits(hwdev, MALIDP_DISP_FUNC_ILACED, MALIDP_DE_DISPLAY_FUNC);
>  	else
>  		malidp_hw_clearbits(hwdev, MALIDP_DISP_FUNC_ILACED, MALIDP_DE_DISPLAY_FUNC);
> +
> +	/*
> +	 *  Program the RQoS register to increasing QoS levels for
> +	 *  the 4k resolution flicker to disappear on the LS1028A.

Looks like two sentences got smashed into one, the result is a bit hard to read.

Best regards,
Liviu

> +	 */
> +	if (hwdev->arqos_high_level) {
> +		val = RED_ARQOS_VALUE | GREEN_ARQOS_VALUE;
> +
> +		if (mode->pixelclock == 594000000)
> +			malidp_hw_setbits(hwdev, val, MALIDP500_RQOS_QUALITY);
> +		else
> +			malidp_hw_clearbits(hwdev, val, MALIDP500_RQOS_QUALITY);
> +	}
>  }
>  
>  int malidp_format_get_bpp(u32 fmt)
> diff --git a/drivers/gpu/drm/arm/malidp_hw.h b/drivers/gpu/drm/arm/malidp_hw.h
> index 968a65eed371..b8baba60508a 100644
> --- a/drivers/gpu/drm/arm/malidp_hw.h
> +++ b/drivers/gpu/drm/arm/malidp_hw.h
> @@ -251,6 +251,9 @@ struct malidp_hw_device {
>  
>  	/* size of memory used for rotating layers, up to two banks available */
>  	u32 rotation_memory[2];
> +
> +	/* priority level of RQOS register used for driven the ARQOS signal */
> +	bool arqos_high_level;
>  };
>  
>  static inline u32 malidp_hw_read(struct malidp_hw_device *hwdev, u32 reg)
> diff --git a/drivers/gpu/drm/arm/malidp_regs.h b/drivers/gpu/drm/arm/malidp_regs.h
> index 993031542fa1..08842142b3b2 100644
> --- a/drivers/gpu/drm/arm/malidp_regs.h
> +++ b/drivers/gpu/drm/arm/malidp_regs.h
> @@ -210,6 +210,18 @@
>  #define MALIDP500_CONFIG_VALID		0x00f00
>  #define MALIDP500_CONFIG_ID		0x00fd4
>  
> +/*
> + * The quality of service (QoS) register on the DP500. RQOS register values
> + * are driven by the ARQOS signal, using AXI transacations, dependent on the
> + * FIFO input level.
> + * The RQOS register can also set QoS levels for:
> + *    - RED_ARQOS   @ A 4-bit signal value for close to underflow conditions
> + *    - GREEN_ARQOS @ A 4-bit signal value for normal conditions
> + */
> +#define MALIDP500_RQOS_QUALITY          0x00500
> +#define RED_ARQOS_VALUE                 (0xd << 12)
> +#define GREEN_ARQOS_VALUE               (0xd << 28)
> +
>  /* register offsets and bits specific to DP550/DP650 */
>  #define MALIDP550_ADDR_SPACE_SIZE	0x10000
>  #define MALIDP550_DE_CONTROL		0x00010
> -- 
> 2.17.1
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

RE: [EXT] Re: [PATCH] drm/arm/mali-dp: Add display QoS interface configuration

[Wen He]

> -----Original Message-----
> From: Liviu Dudau <liviu.dudau@arm.com>
> Sent: 2019年7月17日 19:22
> To: Wen He <wen.he_1@nxp.com>
> Cc: dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org;
> brian.starkey@arm.com; airlied@linux.ie; daniel@ffwll.ch; Leo Li
> <leoyang.li@nxp.com>
> Subject: [EXT] Re: [PATCH] drm/arm/mali-dp: Add display QoS interface
> configuration
> 
> 
> Hi Wen,
> 

Hi Liviu,

> On Wed, Jul 17, 2019 at 05:23:53PM +0800, Wen He wrote:
> > Configure the display Quality of service (QoS) levels to high priority
> > if the level is defined as high as in DTS. The ARQOS for DP500 is
> > driven from the "RQOS" register, needed to program the RQOS register
> > value < 7 for the 4k resolution flicker to disappear on the LS1028A platform.
> 
> Thanks for taking time to come up with a more generic patch for your issue!
> 

Thanks for the review and comments,

> I have a question: what happens if you program the
> MALIDP500_RQOS_QUALITY register to 0xd000d000 for all pixelclocks?
> 

We can't do that, because:
1. The flicker issue only reproduced in 4k@60.
2. This configuration will be reduced DDR benchmark performance data, because the
LCDC and DDR are both to connect CCI-400. we have to make sure that DDR performance
at first when work together with other resolutions.

> Also, some suggestions further down:
> 
> >
> > Signed-off-by: Wen He <wen.he_1@nxp.com>
> > ---
> > change in v2:
> >         - add new implementation for 4k flicker issue on the LS1028A
> >
> >  drivers/gpu/drm/arm/malidp_drv.c  |  5 +++++
> >  drivers/gpu/drm/arm/malidp_hw.c   | 13 +++++++++++++
> >  drivers/gpu/drm/arm/malidp_hw.h   |  3 +++
> >  drivers/gpu/drm/arm/malidp_regs.h | 12 ++++++++++++
> >  4 files changed, 33 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/arm/malidp_drv.c
> > b/drivers/gpu/drm/arm/malidp_drv.c
> > index f25ec4382277..d2b2cf52ac87 100644
> > --- a/drivers/gpu/drm/arm/malidp_drv.c
> > +++ b/drivers/gpu/drm/arm/malidp_drv.c
> > @@ -818,6 +818,11 @@ static int malidp_bind(struct device *dev)
> >
> >       malidp->core_id = version;
> >
> > +     hwdev->arqos_high_level = false;
> 
> This is not needed.
> 
> > +
> > +     hwdev->arqos_high_level = of_property_read_bool(dev->of_node,
> > +                                     "arm,malidp-arqos-high-level");
> 
> I think it would be better to have this property as a u32 value, rather than a
> boolean, and put the value that we want to program MALIDP_RQOS_QUALITY
> with in there.

I really thought that, but I've tested DDR performance for 4k resolution with
different RQOS setting, the best test performance data is when set the RQOS
register value's 0xd000d000. So the value is fixed, I think the Boolean type is
better used here, that's why I did it.

> 
> Also, you need to add the documentation for this optional property in
> Documentation/devicetree/bindings/display/arm,malidp.txt.

Understand, I will generate another patch to add this change for the DT bindings.

> 

> > +
> >       /* set the number of lines used for output of RGB data */
> >       ret = of_property_read_u8_array(dev->of_node,
> >                                       "arm,malidp-output-port-lines",
> > diff --git a/drivers/gpu/drm/arm/malidp_hw.c
> > b/drivers/gpu/drm/arm/malidp_hw.c index 50af399d7f6f..eaa1658cd86b
> > 100644
> > --- a/drivers/gpu/drm/arm/malidp_hw.c
> > +++ b/drivers/gpu/drm/arm/malidp_hw.c
> > @@ -374,6 +374,19 @@ static void malidp500_modeset(struct
> malidp_hw_device *hwdev, struct videomode *
> >               malidp_hw_setbits(hwdev, MALIDP_DISP_FUNC_ILACED,
> MALIDP_DE_DISPLAY_FUNC);
> >       else
> >               malidp_hw_clearbits(hwdev, MALIDP_DISP_FUNC_ILACED,
> > MALIDP_DE_DISPLAY_FUNC);
> > +
> > +     /*
> > +      *  Program the RQoS register to increasing QoS levels for
> > +      *  the 4k resolution flicker to disappear on the LS1028A.
> 
> Looks like two sentences got smashed into one, the result is a bit hard to read.
> 

Ha, maybe should be described like this:
"Program the RQoS register to avoid 4k resolution flicker on the LS1028A."

Best Regards,
Wen

> Best regards,
> Liviu
> 
> > +      */
> > +     if (hwdev->arqos_high_level) {
> > +             val = RED_ARQOS_VALUE | GREEN_ARQOS_VALUE;
> > +
> > +             if (mode->pixelclock == 594000000)
> > +                     malidp_hw_setbits(hwdev, val,
> MALIDP500_RQOS_QUALITY);
> > +             else
> > +                     malidp_hw_clearbits(hwdev, val,
> MALIDP500_RQOS_QUALITY);
> > +     }
> >  }
> >
> >  int malidp_format_get_bpp(u32 fmt)
> > diff --git a/drivers/gpu/drm/arm/malidp_hw.h
> > b/drivers/gpu/drm/arm/malidp_hw.h index 968a65eed371..b8baba60508a
> > 100644
> > --- a/drivers/gpu/drm/arm/malidp_hw.h
> > +++ b/drivers/gpu/drm/arm/malidp_hw.h
> > @@ -251,6 +251,9 @@ struct malidp_hw_device {
> >
> >       /* size of memory used for rotating layers, up to two banks available
> */
> >       u32 rotation_memory[2];
> > +
> > +     /* priority level of RQOS register used for driven the ARQOS signal */
> > +     bool arqos_high_level;
> >  };
> >
> >  static inline u32 malidp_hw_read(struct malidp_hw_device *hwdev, u32
> > reg) diff --git a/drivers/gpu/drm/arm/malidp_regs.h
> > b/drivers/gpu/drm/arm/malidp_regs.h
> > index 993031542fa1..08842142b3b2 100644
> > --- a/drivers/gpu/drm/arm/malidp_regs.h
> > +++ b/drivers/gpu/drm/arm/malidp_regs.h
> > @@ -210,6 +210,18 @@
> >  #define MALIDP500_CONFIG_VALID               0x00f00
> >  #define MALIDP500_CONFIG_ID          0x00fd4
> >
> > +/*
> > + * The quality of service (QoS) register on the DP500. RQOS register
> > +values
> > + * are driven by the ARQOS signal, using AXI transacations, dependent
> > +on the
> > + * FIFO input level.
> > + * The RQOS register can also set QoS levels for:
> > + *    - RED_ARQOS   @ A 4-bit signal value for close to underflow
> conditions
> > + *    - GREEN_ARQOS @ A 4-bit signal value for normal conditions
> > + */
> > +#define MALIDP500_RQOS_QUALITY          0x00500
> > +#define RED_ARQOS_VALUE                 (0xd << 12)
> > +#define GREEN_ARQOS_VALUE               (0xd << 28)
> > +
> >  /* register offsets and bits specific to DP550/DP650 */
> >  #define MALIDP550_ADDR_SPACE_SIZE    0x10000
> >  #define MALIDP550_DE_CONTROL         0x00010
> > --
> > 2.17.1
> >
> 
> --
> ====================
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---------------
>     ¯\_(ツ)_/¯

Re: [EXT] Re: [PATCH] drm/arm/mali-dp: Add display QoS interface configuration

[Liviu Dudau]

On Thu, Jul 18, 2019 at 03:29:48AM +0000, Wen He wrote:
> 
> 
> > -----Original Message-----
> > From: Liviu Dudau <liviu.dudau@arm.com>
> > Sent: 2019年7月17日 19:22
> > To: Wen He <wen.he_1@nxp.com>
> > Cc: dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org;
> > brian.starkey@arm.com; airlied@linux.ie; daniel@ffwll.ch; Leo Li
> > <leoyang.li@nxp.com>
> > Subject: [EXT] Re: [PATCH] drm/arm/mali-dp: Add display QoS interface
> > configuration
> > 
> > 
> > Hi Wen,
> > 
> 
> Hi Liviu,

Hi Wen,

> 
> > On Wed, Jul 17, 2019 at 05:23:53PM +0800, Wen He wrote:
> > > Configure the display Quality of service (QoS) levels to high priority
> > > if the level is defined as high as in DTS. The ARQOS for DP500 is
> > > driven from the "RQOS" register, needed to program the RQOS register
> > > value < 7 for the 4k resolution flicker to disappear on the LS1028A platform.
> > 
> > Thanks for taking time to come up with a more generic patch for your issue!
> > 
> 
> Thanks for the review and comments,
> 
> > I have a question: what happens if you program the
> > MALIDP500_RQOS_QUALITY register to 0xd000d000 for all pixelclocks?
> > 
> 
> We can't do that, because:
> 1. The flicker issue only reproduced in 4k@60.

Can you clarify? Does this mean that with the RQOS setting of 0xd000d000 you
don't see flicker at lower resolutions, or that you haven't tested at other
resolution than 4k@60?

> 2. This configuration will be reduced DDR benchmark performance data, because the
> LCDC and DDR are both to connect CCI-400. we have to make sure that DDR performance
> at first when work together with other resolutions.

Hmm, I'm not sure I'm sharing the same view of the problem. You are writing
into DP500's QoS registers, which don't control how CCI-400 or DDR are going to
behave. Now I agree that a more aggressive QoS from DP500 is going to lead to
more contention to the DDR, but maybe you can look into CCI-400's QoS settings
and tweak the bandwidth allocation there as well.

> 
> > Also, some suggestions further down:
> > 
> > >
> > > Signed-off-by: Wen He <wen.he_1@nxp.com>
> > > ---
> > > change in v2:
> > >         - add new implementation for 4k flicker issue on the LS1028A
> > >
> > >  drivers/gpu/drm/arm/malidp_drv.c  |  5 +++++
> > >  drivers/gpu/drm/arm/malidp_hw.c   | 13 +++++++++++++
> > >  drivers/gpu/drm/arm/malidp_hw.h   |  3 +++
> > >  drivers/gpu/drm/arm/malidp_regs.h | 12 ++++++++++++
> > >  4 files changed, 33 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/arm/malidp_drv.c
> > > b/drivers/gpu/drm/arm/malidp_drv.c
> > > index f25ec4382277..d2b2cf52ac87 100644
> > > --- a/drivers/gpu/drm/arm/malidp_drv.c
> > > +++ b/drivers/gpu/drm/arm/malidp_drv.c
> > > @@ -818,6 +818,11 @@ static int malidp_bind(struct device *dev)
> > >
> > >       malidp->core_id = version;
> > >
> > > +     hwdev->arqos_high_level = false;
> > 
> > This is not needed.
> > 
> > > +
> > > +     hwdev->arqos_high_level = of_property_read_bool(dev->of_node,
> > > +                                     "arm,malidp-arqos-high-level");
> > 
> > I think it would be better to have this property as a u32 value, rather than a
> > boolean, and put the value that we want to program MALIDP_RQOS_QUALITY
> > with in there.
> 
> I really thought that, but I've tested DDR performance for 4k resolution with
> different RQOS setting, the best test performance data is when set the RQOS
> register value's 0xd000d000. So the value is fixed, I think the Boolean type is
> better used here, that's why I did it.

Yes, it is fixed for your platform, but not fixed for other ODMs that might
decide to use the LS1028A chip to create a board. They might use different
DDRs, or their PCB traces might have different lengths. I think it is still
valuable to put the 0xd000d000 value into the device tree and read it from
there. For LS1028A NXP boards it will do then the right thing.

> 
> > 
> > Also, you need to add the documentation for this optional property in
> > Documentation/devicetree/bindings/display/arm,malidp.txt.
> 
> Understand, I will generate another patch to add this change for the DT bindings.
> 
> > 
> 
> > > +
> > >       /* set the number of lines used for output of RGB data */
> > >       ret = of_property_read_u8_array(dev->of_node,
> > >                                       "arm,malidp-output-port-lines",
> > > diff --git a/drivers/gpu/drm/arm/malidp_hw.c
> > > b/drivers/gpu/drm/arm/malidp_hw.c index 50af399d7f6f..eaa1658cd86b
> > > 100644
> > > --- a/drivers/gpu/drm/arm/malidp_hw.c
> > > +++ b/drivers/gpu/drm/arm/malidp_hw.c
> > > @@ -374,6 +374,19 @@ static void malidp500_modeset(struct
> > malidp_hw_device *hwdev, struct videomode *
> > >               malidp_hw_setbits(hwdev, MALIDP_DISP_FUNC_ILACED,
> > MALIDP_DE_DISPLAY_FUNC);
> > >       else
> > >               malidp_hw_clearbits(hwdev, MALIDP_DISP_FUNC_ILACED,
> > > MALIDP_DE_DISPLAY_FUNC);
> > > +
> > > +     /*
> > > +      *  Program the RQoS register to increasing QoS levels for
> > > +      *  the 4k resolution flicker to disappear on the LS1028A.
> > 
> > Looks like two sentences got smashed into one, the result is a bit hard to read.
> > 
> 
> Ha, maybe should be described like this:
> "Program the RQoS register to avoid 4k resolution flicker on the LS1028A."

Yes, that is much easier to understand.

Thank you,
Liviu


> 
> Best Regards,
> Wen
> 
> > Best regards,
> > Liviu
> > 
> > > +      */
> > > +     if (hwdev->arqos_high_level) {
> > > +             val = RED_ARQOS_VALUE | GREEN_ARQOS_VALUE;
> > > +
> > > +             if (mode->pixelclock == 594000000)
> > > +                     malidp_hw_setbits(hwdev, val,
> > MALIDP500_RQOS_QUALITY);
> > > +             else
> > > +                     malidp_hw_clearbits(hwdev, val,
> > MALIDP500_RQOS_QUALITY);
> > > +     }
> > >  }
> > >
> > >  int malidp_format_get_bpp(u32 fmt)
> > > diff --git a/drivers/gpu/drm/arm/malidp_hw.h
> > > b/drivers/gpu/drm/arm/malidp_hw.h index 968a65eed371..b8baba60508a
> > > 100644
> > > --- a/drivers/gpu/drm/arm/malidp_hw.h
> > > +++ b/drivers/gpu/drm/arm/malidp_hw.h
> > > @@ -251,6 +251,9 @@ struct malidp_hw_device {
> > >
> > >       /* size of memory used for rotating layers, up to two banks available
> > */
> > >       u32 rotation_memory[2];
> > > +
> > > +     /* priority level of RQOS register used for driven the ARQOS signal */
> > > +     bool arqos_high_level;
> > >  };
> > >
> > >  static inline u32 malidp_hw_read(struct malidp_hw_device *hwdev, u32
> > > reg) diff --git a/drivers/gpu/drm/arm/malidp_regs.h
> > > b/drivers/gpu/drm/arm/malidp_regs.h
> > > index 993031542fa1..08842142b3b2 100644
> > > --- a/drivers/gpu/drm/arm/malidp_regs.h
> > > +++ b/drivers/gpu/drm/arm/malidp_regs.h
> > > @@ -210,6 +210,18 @@
> > >  #define MALIDP500_CONFIG_VALID               0x00f00
> > >  #define MALIDP500_CONFIG_ID          0x00fd4
> > >
> > > +/*
> > > + * The quality of service (QoS) register on the DP500. RQOS register
> > > +values
> > > + * are driven by the ARQOS signal, using AXI transacations, dependent
> > > +on the
> > > + * FIFO input level.
> > > + * The RQOS register can also set QoS levels for:
> > > + *    - RED_ARQOS   @ A 4-bit signal value for close to underflow
> > conditions
> > > + *    - GREEN_ARQOS @ A 4-bit signal value for normal conditions
> > > + */
> > > +#define MALIDP500_RQOS_QUALITY          0x00500
> > > +#define RED_ARQOS_VALUE                 (0xd << 12)
> > > +#define GREEN_ARQOS_VALUE               (0xd << 28)
> > > +
> > >  /* register offsets and bits specific to DP550/DP650 */
> > >  #define MALIDP550_ADDR_SPACE_SIZE    0x10000
> > >  #define MALIDP550_DE_CONTROL         0x00010
> > > --
> > > 2.17.1
> > >
> > 
> > --
> > ====================
> > | I would like to |
> > | fix the world,  |
> > | but they're not |
> > | giving me the   |
> >  \ source code!  /
> >   ---------------
> >     ¯\_(ツ)_/¯

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

RE: [EXT] Re: [PATCH] drm/arm/mali-dp: Add display QoS interface configuration

[Wen He]

> -----Original Message-----
> From: Liviu Dudau <liviu.dudau@arm.com>
> Sent: 2019年7月18日 17:37
> To: Wen He <wen.he_1@nxp.com>
> Cc: dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org;
> brian.starkey@arm.com; airlied@linux.ie; daniel@ffwll.ch; Leo Li
> <leoyang.li@nxp.com>
> Subject: Re: [EXT] Re: [PATCH] drm/arm/mali-dp: Add display QoS interface
> configuration
> 
> Caution: EXT Email
> 
> On Thu, Jul 18, 2019 at 03:29:48AM +0000, Wen He wrote:
> >
> >
> > > -----Original Message-----
> > > From: Liviu Dudau <liviu.dudau@arm.com>
> > > Sent: 2019年7月17日 19:22
> > > To: Wen He <wen.he_1@nxp.com>
> > > Cc: dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org;
> > > brian.starkey@arm.com; airlied@linux.ie; daniel@ffwll.ch; Leo Li
> > > <leoyang.li@nxp.com>
> > > Subject: [EXT] Re: [PATCH] drm/arm/mali-dp: Add display QoS
> > > interface configuration
> > >
> > >
> > > Hi Wen,
> > >
> >
> > Hi Liviu,
> 
> Hi Wen,
> 
> >
> > > On Wed, Jul 17, 2019 at 05:23:53PM +0800, Wen He wrote:
> > > > Configure the display Quality of service (QoS) levels to high
> > > > priority if the level is defined as high as in DTS. The ARQOS for
> > > > DP500 is driven from the "RQOS" register, needed to program the
> > > > RQOS register value < 7 for the 4k resolution flicker to disappear on the
> LS1028A platform.
> > >
> > > Thanks for taking time to come up with a more generic patch for your
> issue!
> > >
> >
> > Thanks for the review and comments,
> >
> > > I have a question: what happens if you program the
> > > MALIDP500_RQOS_QUALITY register to 0xd000d000 for all pixelclocks?
> > >
> >
> > We can't do that, because:
> > 1. The flicker issue only reproduced in 4k@60.
> 
> Can you clarify? Does this mean that with the RQOS setting of 0xd000d000
> you don't see flicker at lower resolutions, or that you haven't tested at other
> resolution than 4k@60?

I mean that I didn't see flicker issue at lower resolutions without the RQOS setting
of 0xd000d000.

The ROQS register default value's 0x00010001, in this case, only found the flicker
issue can be reproduced in 4k@60, other lower resolutions not found this issue.
So If setting the RQOS register value's to 0xd001d001, 4k flicker issue will be resolve.

> 
> > 2. This configuration will be reduced DDR benchmark performance data,
> > because the LCDC and DDR are both to connect CCI-400. we have to make
> > sure that DDR performance at first when work together with other
> resolutions.
> 
> Hmm, I'm not sure I'm sharing the same view of the problem. You are writing
> into DP500's QoS registers, which don't control how CCI-400 or DDR are going
> to behave. Now I agree that a more aggressive QoS from DP500 is going to
> lead to more contention to the DDR, but maybe you can look into CCI-400's
> QoS settings and tweak the bandwidth allocation there as well.

Yes, I agree with you, but after discussed with our design team, I chose them advice
that only change the DP500's QoS.
Here're comments from our design team:
-------------------
There are multiple registers that impact QOS values in the system, particularly as it
relates to transactions from the LCD Controller.
"The “best” approach (which in theory should result in the LCD Controller having
the bandwidth it needs to avoid buffer underrun, while also having the minimal impact
on the system) is to use the dynamic QoS information (ARQOS) coming from the LCD
Controller. To do this, you need to program the LCD Controller’s “RQOS” register. 
See the attached DP-500 TRM, Section 2.4.6, “Display Quality of Service Interface”
and Section 4.2.9, “Display engine quality of service registers”.
-------------------

> 
> >
> > > Also, some suggestions further down:
> > >
> > > >
> > > > Signed-off-by: Wen He <wen.he_1@nxp.com>
> > > > ---
> > > > change in v2:
> > > >         - add new implementation for 4k flicker issue on the
> > > > LS1028A
> > > >
> > > >  drivers/gpu/drm/arm/malidp_drv.c  |  5 +++++
> > > >  drivers/gpu/drm/arm/malidp_hw.c   | 13 +++++++++++++
> > > >  drivers/gpu/drm/arm/malidp_hw.h   |  3 +++
> > > >  drivers/gpu/drm/arm/malidp_regs.h | 12 ++++++++++++
> > > >  4 files changed, 33 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/arm/malidp_drv.c
> > > > b/drivers/gpu/drm/arm/malidp_drv.c
> > > > index f25ec4382277..d2b2cf52ac87 100644
> > > > --- a/drivers/gpu/drm/arm/malidp_drv.c
> > > > +++ b/drivers/gpu/drm/arm/malidp_drv.c
> > > > @@ -818,6 +818,11 @@ static int malidp_bind(struct device *dev)
> > > >
> > > >       malidp->core_id = version;
> > > >
> > > > +     hwdev->arqos_high_level = false;
> > >
> > > This is not needed.
> > >
> > > > +
> > > > +     hwdev->arqos_high_level = of_property_read_bool(dev->of_node,
> > > > +
> > > > + "arm,malidp-arqos-high-level");
> > >
> > > I think it would be better to have this property as a u32 value,
> > > rather than a boolean, and put the value that we want to program
> > > MALIDP_RQOS_QUALITY with in there.
> >
> > I really thought that, but I've tested DDR performance for 4k
> > resolution with different RQOS setting, the best test performance data
> > is when set the RQOS register value's 0xd000d000. So the value is
> > fixed, I think the Boolean type is better used here, that's why I did it.
> 
> Yes, it is fixed for your platform, but not fixed for other ODMs that might
> decide to use the LS1028A chip to create a board. They might use different
> DDRs, or their PCB traces might have different lengths. I think it is still valuable
> to put the 0xd000d000 value into the device tree and read it from there. For
> LS1028A NXP boards it will do then the right thing.
> 

Hmm, I think you are right, should be use value to instead of the Boolean.

> >
> > >
> > > Also, you need to add the documentation for this optional property
> > > in Documentation/devicetree/bindings/display/arm,malidp.txt.
> >
> > Understand, I will generate another patch to add this change for the DT
> bindings.
> >
> > >
> >
> > > > +
> > > >       /* set the number of lines used for output of RGB data */
> > > >       ret = of_property_read_u8_array(dev->of_node,
> > > >
> > > > "arm,malidp-output-port-lines", diff --git
> > > > a/drivers/gpu/drm/arm/malidp_hw.c
> > > > b/drivers/gpu/drm/arm/malidp_hw.c index 50af399d7f6f..eaa1658cd86b
> > > > 100644
> > > > --- a/drivers/gpu/drm/arm/malidp_hw.c
> > > > +++ b/drivers/gpu/drm/arm/malidp_hw.c
> > > > @@ -374,6 +374,19 @@ static void malidp500_modeset(struct
> > > malidp_hw_device *hwdev, struct videomode *
> > > >               malidp_hw_setbits(hwdev, MALIDP_DISP_FUNC_ILACED,
> > > MALIDP_DE_DISPLAY_FUNC);
> > > >       else
> > > >               malidp_hw_clearbits(hwdev,
> MALIDP_DISP_FUNC_ILACED,
> > > > MALIDP_DE_DISPLAY_FUNC);
> > > > +
> > > > +     /*
> > > > +      *  Program the RQoS register to increasing QoS levels for
> > > > +      *  the 4k resolution flicker to disappear on the LS1028A.
> > >
> > > Looks like two sentences got smashed into one, the result is a bit hard to
> read.
> > >
> >
> > Ha, maybe should be described like this:
> > "Program the RQoS register to avoid 4k resolution flicker on the LS1028A."
> 
> Yes, that is much easier to understand.
> 

Thank you for the detailed review.

Best Regards,
Wen

> Thank you,
> Liviu
> 
> 
> >
> > Best Regards,
> > Wen
> >
> > > Best regards,
> > > Liviu
> > >
> > > > +      */
> > > > +     if (hwdev->arqos_high_level) {
> > > > +             val = RED_ARQOS_VALUE | GREEN_ARQOS_VALUE;
> > > > +
> > > > +             if (mode->pixelclock == 594000000)
> > > > +                     malidp_hw_setbits(hwdev, val,
> > > MALIDP500_RQOS_QUALITY);
> > > > +             else
> > > > +                     malidp_hw_clearbits(hwdev, val,
> > > MALIDP500_RQOS_QUALITY);
> > > > +     }
> > > >  }
> > > >
> > > >  int malidp_format_get_bpp(u32 fmt) diff --git
> > > > a/drivers/gpu/drm/arm/malidp_hw.h
> > > > b/drivers/gpu/drm/arm/malidp_hw.h index
> 968a65eed371..b8baba60508a
> > > > 100644
> > > > --- a/drivers/gpu/drm/arm/malidp_hw.h
> > > > +++ b/drivers/gpu/drm/arm/malidp_hw.h
> > > > @@ -251,6 +251,9 @@ struct malidp_hw_device {
> > > >
> > > >       /* size of memory used for rotating layers, up to two banks
> > > > available
> > > */
> > > >       u32 rotation_memory[2];
> > > > +
> > > > +     /* priority level of RQOS register used for driven the ARQOS signal
> */
> > > > +     bool arqos_high_level;
> > > >  };
> > > >
> > > >  static inline u32 malidp_hw_read(struct malidp_hw_device *hwdev,
> > > > u32
> > > > reg) diff --git a/drivers/gpu/drm/arm/malidp_regs.h
> > > > b/drivers/gpu/drm/arm/malidp_regs.h
> > > > index 993031542fa1..08842142b3b2 100644
> > > > --- a/drivers/gpu/drm/arm/malidp_regs.h
> > > > +++ b/drivers/gpu/drm/arm/malidp_regs.h
> > > > @@ -210,6 +210,18 @@
> > > >  #define MALIDP500_CONFIG_VALID               0x00f00
> > > >  #define MALIDP500_CONFIG_ID          0x00fd4
> > > >
> > > > +/*
> > > > + * The quality of service (QoS) register on the DP500. RQOS
> > > > +register values
> > > > + * are driven by the ARQOS signal, using AXI transacations,
> > > > +dependent on the
> > > > + * FIFO input level.
> > > > + * The RQOS register can also set QoS levels for:
> > > > + *    - RED_ARQOS   @ A 4-bit signal value for close to underflow
> > > conditions
> > > > + *    - GREEN_ARQOS @ A 4-bit signal value for normal conditions
> > > > + */
> > > > +#define MALIDP500_RQOS_QUALITY          0x00500
> > > > +#define RED_ARQOS_VALUE                 (0xd << 12)
> > > > +#define GREEN_ARQOS_VALUE               (0xd << 28)
> > > > +
> > > >  /* register offsets and bits specific to DP550/DP650 */
> > > >  #define MALIDP550_ADDR_SPACE_SIZE    0x10000
> > > >  #define MALIDP550_DE_CONTROL         0x00010
> > > > --
> > > > 2.17.1
> > > >
> > >
> > > --
> > > ====================
> > > | I would like to |
> > > | fix the world,  |
> > > | but they're not |
> > > | giving me the   |
> > >  \ source code!  /
> > >   ---------------
> > >     ¯\_(ツ)_/¯
> 
> --
> ====================
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---------------
>     ¯\_(ツ)_/¯