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

[Frank Oltmanns]

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:
>> > On some pinephones the video output sometimes freezes (flips between two
>> > frames) [1]. It seems to be that the reason for this behaviour is that
>> > PLL-MIPI, PLL-GPU and GPU are operating outside their limits.
>> >
>> > In this patch series I propose the followin changes:
>> >   1. sunxi-ng: Adhere to the following constraints given in the
>> >      Allwinner A64 Manual regarding PLL-MIPI:
>> >       * M/N <= 3
>> >       * (PLL_VIDEO0)/M >= 24MHz
>> >       * 500MHz <= clockrate <= 1400MHz
>> >
>> >   2. Choose a higher clock rate for the ST7703 based XDB599 panel, so
>> >      that the panel function well with the Allwinner A64 SOC. PLL-MIPI
>> >      must run between 500 MHz and 1.4 GHz. As PLL-MIPI runs at 6 times
>> >      the panel's clock rate, we need the panel's clock to be at least
>> >      83.333 MHz.
>> >
>> >   3. Increase the minimum frequency in the A64 DTS OPPs from 120 MHz to
>> >      192 MHz. This further reduces the issue.
>> >
>> > Unfortunately, with these patches the issue [1] is not completely gone,
>> > but becomes less likely.
>> >
>> > Note, that when pinning the GPU to 432 MHz the issue completely
>> > disappears for me. I've searched the BSP and could not find any
>> > indication that supports the idea of having the three OPPs. The only
>> > frequency I found in the BPSs for A64 is 432 MHz, that has also proven
>> > stable for me. So, while increasing the minimum frequency to 192 MHz
>> > reduces the issue, should we maybe instead set the GPU to a fixed 432
>> > MHz instead?
>>
>> Per A64 User Manual 1.1 page 81:
>>
>> (9). Clock output of PLL_GPU can be used for GPU;and dynamic frequency scaling is not supported;
>
> You may be able to elegantly work around this by pinning PLL_GPU to a certain
> frequency (assing it in DT and then don't decalre gpu clock as
> CLK_SET_RATE_PARENT). Say 432 MHz for PLL and then do the scaling via GPU_CLK_REG
> N divider between 432MHz and 216MHz and maybe 108MHz if that brings any gains.
>

I really like this idea. Unfortunately, it seems that the divisor of the
GPU_CLK_REG must always be set to 1. I tried to convey this info in the
commit message for PATCH 6. If using a different divisor than 1 the bug
I'm trying to fix here occurs within minutes. Nevertheless, I gave your
proposal a quick try - it seems so elegant. Unfortunately, but not
unexpectedly, the bug occured almost immediately.

> Then you can perhaps sidestep all these potential issues with PLL_GPU and
> lack of DVFS support decalred in the manual.
>
> regards,
> 	o.
>
>> 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?

>> 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

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>
>> >