[PATCH] rockchip: rk3328: Set VOP QoS to high priority

[PATCH] rockchip: rk3328: Set VOP QoS to high priority

[Nicolas Frattaroli]

The default priority for the quality of service for the video
output results in unsightly glitches on the output whenever there
is memory pressure on the system, which happens a lot.

This sets the VOP QoS to high priority, which fixes this issue.

Signed-off-by: Nicolas Frattaroli <frattaroli.nicolas@gmail.com>
---
 arch/arm/mach-rockchip/rk3328/rk3328.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/mach-rockchip/rk3328/rk3328.c b/arch/arm/mach-rockchip/rk3328/rk3328.c
index de17b88682..2373586b14 100644
--- a/arch/arm/mach-rockchip/rk3328/rk3328.c
+++ b/arch/arm/mach-rockchip/rk3328/rk3328.c
@@ -19,6 +19,8 @@ DECLARE_GLOBAL_DATA_PTR;
 #define GRF_BASE		0xFF100000
 #define UART2_BASE		0xFF130000
 #define FW_DDR_CON_REG		0xFF7C0040
+#define QOS_VOP_OFFSET		0xFF760080
+#define QOS_VOP_PRIORITY	0x8
 
 const char * const boot_devices[BROM_LAST_BOOTSOURCE + 1] = {
 	[BROM_BOOTSOURCE_EMMC] = "/mmc@ff520000",
@@ -54,6 +56,9 @@ int arch_cpu_init(void)
 
 	/* Disable the ddr secure region setting to make it non-secure */
 	rk_setreg(FW_DDR_CON_REG, 0x200);
+#else
+	printf("Setting VOP QoS\n");
+	rk_setreg(QOS_VOP_OFFSET + QOS_VOP_PRIORITY, 0x2);
 #endif
 	return 0;
 }
-- 
2.37.1

Re: [PATCH] rockchip: rk3328: Set VOP QoS to high priority

[Nicolas Frattaroli]

On Samstag, 23. Juli 2022 13:28:36 CEST Nicolas Frattaroli wrote:
> The default priority for the quality of service for the video
> output results in unsightly glitches on the output whenever there
> is memory pressure on the system, which happens a lot.
> 
> This sets the VOP QoS to high priority, which fixes this issue.
> 
> Signed-off-by: Nicolas Frattaroli <frattaroli.nicolas@gmail.com>
> ---
>  arch/arm/mach-rockchip/rk3328/rk3328.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm/mach-rockchip/rk3328/rk3328.c b/arch/arm/mach-rockchip/rk3328/rk3328.c
> index de17b88682..2373586b14 100644
> --- a/arch/arm/mach-rockchip/rk3328/rk3328.c
> +++ b/arch/arm/mach-rockchip/rk3328/rk3328.c
> @@ -19,6 +19,8 @@ DECLARE_GLOBAL_DATA_PTR;
>  #define GRF_BASE		0xFF100000
>  #define UART2_BASE		0xFF130000
>  #define FW_DDR_CON_REG		0xFF7C0040
> +#define QOS_VOP_OFFSET		0xFF760080
> +#define QOS_VOP_PRIORITY	0x8
>  
>  const char * const boot_devices[BROM_LAST_BOOTSOURCE + 1] = {
>  	[BROM_BOOTSOURCE_EMMC] = "/mmc@ff520000",
> @@ -54,6 +56,9 @@ int arch_cpu_init(void)
>  
>  	/* Disable the ddr secure region setting to make it non-secure */
>  	rk_setreg(FW_DDR_CON_REG, 0x200);
> +#else
> +	printf("Setting VOP QoS\n");
> +	rk_setreg(QOS_VOP_OFFSET + QOS_VOP_PRIORITY, 0x2);
>  #endif
>  	return 0;
>  }
> 

Hello,

could I get a review on this?

Thank you for your time,
Nicolas Frattaroli

Re: [PATCH] rockchip: rk3328: Set VOP QoS to high priority

[Nicolas Frattaroli]

On Freitag, 21. Oktober 2022 14:30:39 CET Nicolas Frattaroli wrote:
> On Samstag, 23. Juli 2022 13:28:36 CEST Nicolas Frattaroli wrote:
> > The default priority for the quality of service for the video
> > output results in unsightly glitches on the output whenever there
> > is memory pressure on the system, which happens a lot.
> > 
> > This sets the VOP QoS to high priority, which fixes this issue.
> > 
> > Signed-off-by: Nicolas Frattaroli <frattaroli.nicolas@gmail.com>
> > ---
> >  arch/arm/mach-rockchip/rk3328/rk3328.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/arch/arm/mach-rockchip/rk3328/rk3328.c b/arch/arm/mach-rockchip/rk3328/rk3328.c
> > index de17b88682..2373586b14 100644
> > --- a/arch/arm/mach-rockchip/rk3328/rk3328.c
> > +++ b/arch/arm/mach-rockchip/rk3328/rk3328.c
> > @@ -19,6 +19,8 @@ DECLARE_GLOBAL_DATA_PTR;
> >  #define GRF_BASE		0xFF100000
> >  #define UART2_BASE		0xFF130000
> >  #define FW_DDR_CON_REG		0xFF7C0040
> > +#define QOS_VOP_OFFSET		0xFF760080
> > +#define QOS_VOP_PRIORITY	0x8
> >  
> >  const char * const boot_devices[BROM_LAST_BOOTSOURCE + 1] = {
> >  	[BROM_BOOTSOURCE_EMMC] = "/mmc@ff520000",
> > @@ -54,6 +56,9 @@ int arch_cpu_init(void)
> >  
> >  	/* Disable the ddr secure region setting to make it non-secure */
> >  	rk_setreg(FW_DDR_CON_REG, 0x200);
> > +#else
> > +	printf("Setting VOP QoS\n");
> > +	rk_setreg(QOS_VOP_OFFSET + QOS_VOP_PRIORITY, 0x2);
> >  #endif
> >  	return 0;
> >  }
> > 
> 
> Hello,
> 
> could I get a review on this?
> 
> Thank you for your time,
> Nicolas Frattaroli

It has been almost 6 months since patch submission now.

Re: [PATCH] rockchip: rk3328: Set VOP QoS to high priority

[Jagan Teki]

On Sat, Jul 23, 2022 at 10:31 PM Nicolas Frattaroli
<frattaroli.nicolas@gmail.com> wrote:
>
> The default priority for the quality of service for the video
> output results in unsightly glitches on the output whenever there
> is memory pressure on the system, which happens a lot.
>
> This sets the VOP QoS to high priority, which fixes this issue.

Can you point out how it reproduces?

Jagan.

Re: [PATCH] rockchip: rk3328: Set VOP QoS to high priority

[Nicolas Frattaroli]

On Mittwoch, 4. Januar 2023 12:21:35 CET Jagan Teki wrote:
> On Sat, Jul 23, 2022 at 10:31 PM Nicolas Frattaroli
> <frattaroli.nicolas@gmail.com> wrote:
> >
> > The default priority for the quality of service for the video
> > output results in unsightly glitches on the output whenever there
> > is memory pressure on the system, which happens a lot.
> >
> > This sets the VOP QoS to high priority, which fixes this issue.
> 
> Can you point out how it reproduces?
> 
> Jagan.
> 

To reproduce, run something like glmark2-es2-drm while also
running stress --vm 4, you should see glitchy output on the
HDMI on rk3328.

A more real-world test is to just run a desktop environment,
you will see horizontal lines through the screen the moment
you start typing in e.g. a terminal emulator.

Cheers,
Nicolas Frattaroli

Re: [PATCH] rockchip: rk3328: Set VOP QoS to high priority

[Philipp Tomsich]

On Sat, 23 Jul 2022 at 13:29, Nicolas Frattaroli
<frattaroli.nicolas@gmail.com> wrote:
>
> The default priority for the quality of service for the video
> output results in unsightly glitches on the output whenever there
> is memory pressure on the system, which happens a lot.
>
> This sets the VOP QoS to high priority, which fixes this issue.
>
> Signed-off-by: Nicolas Frattaroli <frattaroli.nicolas@gmail.com>
> ---
>  arch/arm/mach-rockchip/rk3328/rk3328.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/arch/arm/mach-rockchip/rk3328/rk3328.c b/arch/arm/mach-rockchip/rk3328/rk3328.c
> index de17b88682..2373586b14 100644
> --- a/arch/arm/mach-rockchip/rk3328/rk3328.c
> +++ b/arch/arm/mach-rockchip/rk3328/rk3328.c
> @@ -19,6 +19,8 @@ DECLARE_GLOBAL_DATA_PTR;
>  #define GRF_BASE               0xFF100000
>  #define UART2_BASE             0xFF130000
>  #define FW_DDR_CON_REG         0xFF7C0040
> +#define QOS_VOP_OFFSET         0xFF760080
> +#define QOS_VOP_PRIORITY       0x8
>
>  const char * const boot_devices[BROM_LAST_BOOTSOURCE + 1] = {
>         [BROM_BOOTSOURCE_EMMC] = "/mmc@ff520000",
> @@ -54,6 +56,9 @@ int arch_cpu_init(void)
>
>         /* Disable the ddr secure region setting to make it non-secure */
>         rk_setreg(FW_DDR_CON_REG, 0x200);
> +#else
> +       printf("Setting VOP QoS\n");
> +       rk_setreg(QOS_VOP_OFFSET + QOS_VOP_PRIORITY, 0x2);

The change itself is easy enough to give a Reviewed-by on for (yes,
the address is correct, and 0x2 is a valid setting — please use a
symbolic constant for the 0x2, though), but...

Thinking about why you put this into '#else' had me question whether
this indeed belongs here...
This is part of the SoC init, as it sets up the arbitration in the
NOC: so at least whether arch_cpu_init is the right place to have it
is debatable (or if it should go into its own driver).

However, the overarching question is whether this should only be done
as part of the system configuration under the control of other drivers
(e.g., the Linux VOP driver) or under the control of the system
designer (i.e., based on device-tree or /proc entries in Linux).
Consider the case where someone is building a network/storage
appliance that uses VOP for console output: in such an application,
priority would be given to the peripherals critical to that
application instead of VOP.

So, I'd like to hear more discussion on whether this should be
unconditionally done in the bootloader before we move forward.

Thanks,
Philipp.

>  #endif
>         return 0;
>  }
> --
> 2.37.1
>

Re: [PATCH] rockchip: rk3328: Set VOP QoS to high priority

[Kever Yang]

Hi Nicolas,

     I can understand this change can fix the problem you have met.

     But this is likely a board specific setting instead of a soc 
platform setting, rockchip kernel have a qos dts node

to set to qos priority for different IP master and this can be customize 
for different board.


Thanks,

- Kever

On 2022/7/23 19:28, Nicolas Frattaroli wrote:
> The default priority for the quality of service for the video
> output results in unsightly glitches on the output whenever there
> is memory pressure on the system, which happens a lot.
>
> This sets the VOP QoS to high priority, which fixes this issue.
>
> Signed-off-by: Nicolas Frattaroli <frattaroli.nicolas@gmail.com>
> ---
>   arch/arm/mach-rockchip/rk3328/rk3328.c | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/arch/arm/mach-rockchip/rk3328/rk3328.c b/arch/arm/mach-rockchip/rk3328/rk3328.c
> index de17b88682..2373586b14 100644
> --- a/arch/arm/mach-rockchip/rk3328/rk3328.c
> +++ b/arch/arm/mach-rockchip/rk3328/rk3328.c
> @@ -19,6 +19,8 @@ DECLARE_GLOBAL_DATA_PTR;
>   #define GRF_BASE		0xFF100000
>   #define UART2_BASE		0xFF130000
>   #define FW_DDR_CON_REG		0xFF7C0040
> +#define QOS_VOP_OFFSET		0xFF760080
> +#define QOS_VOP_PRIORITY	0x8
>   
>   const char * const boot_devices[BROM_LAST_BOOTSOURCE + 1] = {
>   	[BROM_BOOTSOURCE_EMMC] = "/mmc@ff520000",
> @@ -54,6 +56,9 @@ int arch_cpu_init(void)
>   
>   	/* Disable the ddr secure region setting to make it non-secure */
>   	rk_setreg(FW_DDR_CON_REG, 0x200);
> +#else
> +	printf("Setting VOP QoS\n");
> +	rk_setreg(QOS_VOP_OFFSET + QOS_VOP_PRIORITY, 0x2);
>   #endif
>   	return 0;
>   }