Re: [PATCH v2 0/6] Pinephone video out fixes (flipping between two frames)

[Ondřej Jirman]

Hi Frank,

On Sun, Feb 11, 2024 at 04:09:16PM +0100, Frank Oltmanns wrote:
> Hi Ondřej,
> 
> On 2024-02-05 at 17:02:00 +0100, Ondřej Jirman <megi@xxxxxx> wrote:
> > On Mon, Feb 05, 2024 at 04:54:07PM +0100, Ondřej Jirman wrote:
> >> On Mon, Feb 05, 2024 at 04:22:23PM +0100, Frank Oltmanns wrote:
> >>
> >> [...]
> >>
> >> Also sunxi-ng clk driver does apply NM factors at once to PLL_GPU clock,
> >> which can cause sudden frequency increase beyond intended output frequency,
> >> because division is applied immediately while multiplication is reflected
> >> slowly.
> >>
> >> Eg. if you're changing divider from 7 to 1, you can get a sudden 7x output
> >> frequency spike, before PLL VCO manages to lower the frequency through N clk
> >> divider feedback loop and lock on again. This can mess up whatever's connected
> >> to the output quite badly.
> >>
> >> You'd have to put logging on kernel writes to PLL_GPU register to see what
> >> is written in there and if divider is lowered significantly on some GPU
> >> devfreq frequency transitions.
> 
> By looking at the clocks in clk_summary in debugfs, the rate of PLL-GPU
> always matches the rate of the GPU (at least at 120, 312, and 432 MHz).
> This is further underlined by the fact, that none of the rates can be
> achieved by integer dividing one of the other rates. sunxi-ng would
> only favor a different rate for pll-gpu than the one that is requested
> for the gpu, if pll-gpu is already running at a rate such that there
> exists an M ∈ {1, 2, 3, 4, 5, 6, 7, 8}, where
>   rate of pll-gpu / M = requested gpu rate
> or if the requested rate could not be reached directly by pll-gpu. Both
> is not the case for the rates in question (120, 192, 312, and 432 MHz).
> 
> This means that the following divisor/multipliers are used by sunxi-ng's
> ccu_nm:
> N =  5, M = 1 for 120 MHz (min value without PATCH 6)
> N =  8, M = 1 for 192 MHz (min value after applying PATCH 6)
> N = 13, M = 1 for 312 MHz
> N = 18, M = 1 for 432 MHz
> 
> So, with or without PATCH 6, the divider stays constant and it's only
> the multiplier that changes. This means, there should be no unexpected
> frequency spikes, right?

Maybe. Thanks for giving it a try. There may still be other kinds of glitches
even if the divisor stays the same. It all depends how the register update is
implemented in the PLL block. It's hard to say. I guess, unless Allwinner
guarantees glitchless output from a given PLL when changing its parameters,
you can't rely on the output being clean during changes.

> >> It's also unclear what happens when FRAC_CLK_OUT or PLL_MODE_SEL changes.
> 
> Those are not changed once the clock is initialized. The bug however
> occurs hours or days after booting. IMO, this makes it unlikely that this
> could be the culprit.
> 
> >> Maybe not much because M is supposed to be set to 1, but you still need to
> >> care when enabling fractional mode, and setting M to 1 because that's exactly
> >> the bad scenario if M was previously higher than 1.
> >>
> >> It's tricky.
> >>
> >> Having GPU module clock gated during PLL config changes may help! You can
> >> do that without locking yourself out, unlike with the CPU PLL.
> >>
> >> There's a gate enable bit for it at GPU_CLK_REG.SCLK_GATING. (page 122)
> 
> The GPU should already be properly gated:
> https://elixir.bootlin.com/linux/v6.7.4/source/drivers/clk/sunxi-ng/ccu-sun50i-a64.c#L599

How so? That's just clock declaration. How does it guarantee the clock to the
module is gated during parent PLL configuration changes?

CLK_SET_RATE_PARENT only gates output on re-parenting, not on parent rate changes,
according to the header:

  https://elixir.bootlin.com/linux/v6.7.4/source/include/linux/clk-provider.h#L19

You'd need perhaps CLK_SET_RATE_GATE *and* still verify that it actually works
as expected via some tracing of gpu clock enable/disable/set_rate and pll-gpu
set_rate. CLK_SET_RATE_GATE seems confusingly docummented:

  https://elixir.bootlin.com/linux/v6.7.4/source/drivers/clk/clk.c#L1034

so I don't particularly trust it does exaclty what the header claims and what
would be needed to test the theory that gating gpu clock during rate change
might help.

kind regards,
	o.

> Thank you for your detailed proposal! It was insightful to read. But
> while those were all great ideas, they have all already been taken care
> of. I'm fresh out of ideas again (except for pinning the GPU rate).
> 
> Again, thank you so much,
>   Frank
> 
> >>
> >> Kind regards,
> >> 	o.
> >>
> >> > I very much appreciate your feedback!
> >> >
> >> > [1] https://gitlab.com/postmarketOS/pmaports/-/issues/805
> >> >
> >> > Signed-off-by: Frank Oltmanns <frank@xxxxxxxxxxxx>
> >> > ---
> >> > Changes in v2:
> >> > - dts: Increase minimum GPU frequency to 192 MHz.
> >> > - nkm and a64: Add minimum and maximum rate for PLL-MIPI.
> >> > - nkm: Use the same approach for skipping invalid rates in
> >> >   ccu_nkm_find_best() as in ccu_nkm_find_best_with_parent_adj().
> >> > - nkm: Improve names for ratio struct members and hence get rid of
> >> >   describing comments.
> >> > - nkm and a64: Correct description in the commit messages: M/N <= 3
> >> > - Remove patches for nm as they were not needed.
> >> > - st7703: Rework the commit message to cover more background for the
> >> >   change.
> >> > - Link to v1: https://lore.kernel.org/r/20231218-pinephone-pll-fixes-v1-0-e238b6ed6dc1@xxxxxxxxxxxx
> >> >
> >> > ---
> >> > Frank Oltmanns (6):
> >> >       clk: sunxi-ng: nkm: Support constraints on m/n ratio and parent rate
> >> >       clk: sunxi-ng: a64: Add constraints on PLL-MIPI's n/m ratio and parent rate
> >> >       clk: sunxi-ng: nkm: Support minimum and maximum rate
> >> >       clk: sunxi-ng: a64: Set minimum and maximum rate for PLL-MIPI
> >> >       drm/panel: st7703: Drive XBD599 panel at higher clock rate
> >> >       arm64: dts: allwinner: a64: Fix minimum GPU OPP rate
> >> >
> >> >  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi |  4 ++--
> >> >  drivers/clk/sunxi-ng/ccu-sun50i-a64.c         | 14 +++++++----
> >> >  drivers/clk/sunxi-ng/ccu_nkm.c                | 34 +++++++++++++++++++++++++++
> >> >  drivers/clk/sunxi-ng/ccu_nkm.h                |  4 ++++
> >> >  drivers/gpu/drm/panel/panel-sitronix-st7703.c | 14 +++++------
> >> >  5 files changed, 56 insertions(+), 14 deletions(-)
> >> > ---
> >> > base-commit: 059c53e877ca6e723e10490c27c1487a63e66efe
> >> > change-id: 20231218-pinephone-pll-fixes-0ccdfde273e4
> >> >
> >> > Best regards,
> >> > --
> >> > Frank Oltmanns <frank@xxxxxxxxxxxx>
> >> >