* Re: [PATCH] rockchip: rk3328: Set VOP QoS to high priority
2022-07-23 11:28 [PATCH] rockchip: rk3328: Set VOP QoS to high priority Nicolas Frattaroli
@ 2022-10-21 12:30 ` Nicolas Frattaroli
2023-01-04 11:18 ` Nicolas Frattaroli
2023-01-04 11:21 ` Jagan Teki
` (2 subsequent siblings)
3 siblings, 1 reply; 7+ messages in thread
From: Nicolas Frattaroli @ 2022-10-21 12:30 UTC (permalink / raw)
To: Simon Glass, Philipp Tomsich, Kever Yang; +Cc: u-boot
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] rockchip: rk3328: Set VOP QoS to high priority
2022-10-21 12:30 ` Nicolas Frattaroli
@ 2023-01-04 11:18 ` Nicolas Frattaroli
0 siblings, 0 replies; 7+ messages in thread
From: Nicolas Frattaroli @ 2023-01-04 11:18 UTC (permalink / raw)
To: Simon Glass, Philipp Tomsich, Kever Yang; +Cc: u-boot
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] rockchip: rk3328: Set VOP QoS to high priority
2022-07-23 11:28 [PATCH] rockchip: rk3328: Set VOP QoS to high priority Nicolas Frattaroli
2022-10-21 12:30 ` Nicolas Frattaroli
@ 2023-01-04 11:21 ` Jagan Teki
2023-01-04 11:30 ` Nicolas Frattaroli
2023-01-04 12:04 ` Philipp Tomsich
2023-01-05 2:19 ` Kever Yang
3 siblings, 1 reply; 7+ messages in thread
From: Jagan Teki @ 2023-01-04 11:21 UTC (permalink / raw)
To: Nicolas Frattaroli; +Cc: Simon Glass, Philipp Tomsich, Kever Yang, u-boot
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] rockchip: rk3328: Set VOP QoS to high priority
2023-01-04 11:21 ` Jagan Teki
@ 2023-01-04 11:30 ` Nicolas Frattaroli
0 siblings, 0 replies; 7+ messages in thread
From: Nicolas Frattaroli @ 2023-01-04 11:30 UTC (permalink / raw)
To: Jagan Teki; +Cc: Simon Glass, Philipp Tomsich, Kever Yang, u-boot
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] rockchip: rk3328: Set VOP QoS to high priority
2022-07-23 11:28 [PATCH] rockchip: rk3328: Set VOP QoS to high priority Nicolas Frattaroli
2022-10-21 12:30 ` Nicolas Frattaroli
2023-01-04 11:21 ` Jagan Teki
@ 2023-01-04 12:04 ` Philipp Tomsich
2023-01-05 2:19 ` Kever Yang
3 siblings, 0 replies; 7+ messages in thread
From: Philipp Tomsich @ 2023-01-04 12:04 UTC (permalink / raw)
To: Nicolas Frattaroli; +Cc: Simon Glass, Kever Yang, u-boot
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] rockchip: rk3328: Set VOP QoS to high priority
2022-07-23 11:28 [PATCH] rockchip: rk3328: Set VOP QoS to high priority Nicolas Frattaroli
` (2 preceding siblings ...)
2023-01-04 12:04 ` Philipp Tomsich
@ 2023-01-05 2:19 ` Kever Yang
3 siblings, 0 replies; 7+ messages in thread
From: Kever Yang @ 2023-01-05 2:19 UTC (permalink / raw)
To: Nicolas Frattaroli, Simon Glass, Philipp Tomsich; +Cc: u-boot
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;
> }
^ permalink raw reply [flat|nested] 7+ messages in thread