Re: [PATCH v2 13/14] [DO NOT MERGE] arm64: dts: allwinner: h6: Add GPU OPP table

[Clément Péron]

Hi Maxime and All,

On Sat, 4 Jul 2020 at 16:56, Clément Péron <peron...@gmail.com> wrote:
>
> Hi Maxime,
>
> On Sat, 4 Jul 2020 at 14:13, Maxime Ripard <max...@cerno.tech> wrote:
> >
> > Hi,
> >
> > On Sat, Jul 04, 2020 at 12:25:34PM +0200, Clément Péron wrote:
> > > Add an Operating Performance Points table for the GPU to
> > > enable Dynamic Voltage & Frequency Scaling on the H6.
> > >
> > > The voltage range is set with minival voltage set to the target
> > > and the maximal voltage set to 1.2V. This allow DVFS framework to
> > > work properly on board with fixed regulator.
> > >
> > > Signed-off-by: Clément Péron <peron...@gmail.com>
> >
> > That patch seems reasonable, why shouldn't we merge it?
>
> I didn't test it a lot and last time I did, some frequencies looked unstable.
> https://lore.kernel.org/patchwork/cover/1239739/
>
> This series adds regulator support to Panfrost devfreq, I will send a
> new one if DVFS on the H6 GPU is stable.
>
> I got this running glmark2 last time
> # glmark2-es2-drm
> =======================================================
> glmark2 2017.07
> =======================================================
> OpenGL Information
> GL_VENDOR: Panfrost
> GL_RENDERER: Mali T720 (Panfrost)
> GL_VERSION: OpenGL ES 2.0 Mesa 20.0.5
> =======================================================
>
> [ 93.550063] panfrost 1800000.gpu: GPU Fault 0x00000088 (UNKNOWN) at
> 0x0000000080117100
> [ 94.045401] panfrost 1800000.gpu: gpu sched timeout, js=0,
> config=0x3700, status=0x8, head=0x21d6c00, tail=0x21d6c00,
> sched_job=00000000e3c2132f
>
> [ 328.871070] panfrost 1800000.gpu: Unhandled Page fault in AS0 at VA
> 0x0000000000000000
> [ 328.871070] Reason: TODO
> [ 328.871070] raw fault status: 0xAA0003C2
> [ 328.871070] decoded fault status: SLAVE FAULT
> [ 328.871070] exception type 0xC2: TRANSLATION_FAULT_LEVEL2
> [ 328.871070] access type 0x3: WRITE
> [ 328.871070] source id 0xAA00
> [ 329.373327] panfrost 1800000.gpu: gpu sched timeout, js=1,
> config=0x3700, status=0x8, head=0xa1a4900, tail=0xa1a4900,
> sched_job=000000007ac31097
> [ 329.386527] panfrost 1800000.gpu: js fault, js=0,
> status=DATA_INVALID_FAULT, head=0xa1a4c00, tail=0xa1a4c00
> [ 329.396293] panfrost 1800000.gpu: gpu sched timeout, js=0,
> config=0x3700, status=0x58, head=0xa1a4c00, tail=0xa1a4c00,
> sched_job=0000000004c90381
> [ 329.411521] panfrost 1800000.gpu: Unhandled Page fault in AS0 at VA
> 0x0000000000000000
> [ 329.411521] Reason: TODO
> [ 329.411521] raw fault status: 0xAA0003C2
> [ 329.411521] decoded fault status: SLAVE FAULT
> [ 329.411521] exception type 0xC2: TRANSLATION_FAULT_LEVEL2
> [ 329.411521] access type 0x3: WRITE
> [ 329.411521] source id 0xAA00

Just to keep a track of this issue.

Piotr Oniszczuk give more test and seems to be software related:
https://www.spinics.net/lists/dri-devel/msg264279.html

Ondrej gave a great explanation about a possible origin of this issue:
https://freenode.irclog.whitequark.org/linux-sunxi/2020-07-11

20:12 <megi> looks like gpu pll on H6 is NKMP clock, and those are
implemented in such a way in mainline that they are prone to
overshooting the frequency during output divider reduction
20:13 <megi> so disabling P divider may help
20:13 <megi> or fixing the dividers
20:14 <megi> and just allowing N to change
20:22 <megi> hmm, I haven't looked at this for quite some time, but H6
BSP way of setting PLL factors actually makes the most sense out of
everything I've seen/tested so far
20:23 <megi> it waits for lock not after setting NK factors, but after
reducing the M factor (pre-divider)
20:24 <megi> I might as well re-run my CPU PLL tester with this
algorithm, to see if it fixes the lockups
20:26 <megi> it makes sense to wait for PLL to stabilize "after"
changing all the factors that actually affect the VCO, and not just
some of them
20:27 <megi> warpme_: ^
20:28 <megi> it may be the same thing that plagues the CPU PLL rate
changes at runtime

Regards,
Clement

>
> Regards,
> Clement
>
> >
> > > ---
> > > arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 80 ++++++++++++++++++++
> > > 1 file changed, 80 insertions(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> > > index 8f514a2169aa..a69f9e09a829 100644
> > > --- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> > > @@ -174,6 +174,7 @@ gpu: gpu@1800000 {
> > > clocks = <&ccu CLK_GPU>, <&ccu CLK_BUS_GPU>;
> > > clock-names = "core", "bus";
> > > resets = <&ccu RST_BUS_GPU>;
> > > + operating-points-v2 = <&gpu_opp_table>;
> > > #cooling-cells = <2>;
> > > status = "disabled";
> > > };
> > > @@ -1036,4 +1037,83 @@ map0 {
> > > };
> > > };
> > > };
> > > +
> > > + gpu_opp_table: gpu-opp-table {
> > > + compatible = "operating-points-v2";
> > > +
> > > + opp@216000000 {
> > > + opp-hz = /bits/ 64 <216000000>;
> > > + opp-microvolt = <810000 810000 1200000>;
> > > + };
> >
> > All those nodes will create DTC warnings though.
> >
> > Maxime

[Clément Péron]

Hi Maxime,

On Tue, 25 Aug 2020 at 15:35, Maxime Ripard <max...@cerno.tech> wrote:
>
> Hi Clement,

> I guess it's one of the bugs we never heard of...
>
> It would be a good idea to test it on another platform (like Rockchip?)
> to rule out any driver issue?
>
> What do you think?

I can't exclude a bug in the driver, but if it was the case LE
community or Panfrost maintainer would have heard of that.

Megi's explanations match what I observed.
NKMP drivers seem the perfect guilty here or maybe it's a combination of both...

Jernej sent me this patch to test:
https://github.com/clementperon/linux/commit/56bde359beaf8e827ce53ede1fe4a0ad233cb79b
But it didn't fix the issue, If someone want to have a look at it :)

Regards,
Clement

>
> Maxime

[Ondřej Jirman]

Not sure how that patch is supposed to work, but it seems to apply
all factors at once to me.

regards,
o.

> Regards,
> Clement
>
> >
> > Maxime

[Jernej Škrabec]

Dne petek, 28. avgust 2020 ob 14:21:19 CEST je Ondřej Jirman napisal(a):

It's hackish but in essence, it is what vendor clock driver does. From A83T or
so onwards, vendor driver uses "lock_mode = PLL_LOCK_NEW_MODE" and this hack
tries to mimick that locking behaviour (good enough for tests).

And yes, this code still applies all factors at once, because vendor driver
does that too on H6. Only case where vendor driver applies factors one by one
is for V3s SoC.

Btw, only recently I noticed following block in h6 clk driver:
https://elixir.bootlin.com/linux/latest/source/drivers/clk/sunxi-ng/ccu-sun50i-h6.c#L1196

Other clk drivers don't have it. Maybe it has some influence in this matter.

Best regards,
Jernej