Re: [PATCH 5/5] drm/panel: st7703: Drive XBD599 panel at higher clock rate

[Frank Oltmanns <frank@oltmanns.dev>]

On 2023-12-20 at 16:18:49 +0100, Jernej Škrabec <jernej.skrabec@gmail.com> wrote:
> Dne sreda, 20. december 2023 ob 08:14:27 CET je Frank Oltmanns napisal(a):
>>
>> On 2023-12-19 at 18:04:29 +0100, Jernej Škrabec <jernej.skrabec@gmail.com> wrote:
>> > Dne ponedeljek, 18. december 2023 ob 14:35:23 CET je Frank Oltmanns napisal(a):
>> >> This panel is used in the pinephone that runs on a Allwinner A64 SOC.
>> >> Acoording to it's datasheet, the SOC requires PLL-MIPI to run at more
>> >> than 500 MHz.
>> >>
>> >> Therefore, change [hv]sync_(start|end) so that we reach a clock rate
>> >> that is high enough to drive PLL-MIPI within its limits.
>> >>
>> >> Signed-off-by: Frank Oltmanns <frank@oltmanns.dev>
>> >
>> > I'm not too sure about this patch. I see that PLL_MIPI doesn't have set
>> > minimum frequency limit in clock driver. If you add it, clock framework
>> > should find rate that is high enough and divisible with target rate.
>>
>> This one is really a tough nut. Unfortunately, the PLL_MIPI clock for
>> this panel has to run exactly at 6 * panel clock. Let me start by
>> showing the relevant part of the clock tree (this is on the pinephone
>> after applying the patches):
>>     pll-video0                 393600000
>>        pll-mipi                500945454
>>           tcon0                500945454
>>              tcon-data-clock   125236363
>>
>> To elaborate, tcon-data-clock has to run at 1/4 the DSI per-lane bit
>> rate [1]. It's a fixed divisor
>>
>> The panel I'm proposing to change is defined as this:
>>
>>     static const struct st7703_panel_desc xbd599_desc = {
>>     	.mode = &xbd599_mode,
>>     	.lanes = 4,
>>     	.mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE,
>>     	.format = MIPI_DSI_FMT_RGB888,
>>     	.init_sequence = xbd599_init_sequence,
>>     };
>>
>> So, we have 24 bpp and 4 lanes. Therefore, the resulting requested
>> tcon-data-clock rate is
>>     crtc_clock * 1000 * (24 / 4) / 4
>>
>> tcon-data-clock therefore requests a parent rate of
>>     4 * (crtc_clock * 1000 * (24 / 4) / 4)
>>
>> The initial 4 is the fixed divisor between tcon0 and tcon-data-clock.
>> Since tcon0 is a ccu_mux, the rate of tcon0 equals the rate of pll-mipi.
>>
>> Since PLL-MIPI has to run at at least at 500MHz this forces us to have a
>> crtc_clock >= 83.333 MHz. The mode I'm prorposing results in a rate of
>> 83.502 MHz.
>
> This is much better explanation why this change is needed. Still, I think
> adding min and max rate to PLL_MIPI would make sense, so proper rates
> are guaranteed.

Okay, I'll include min and max rate in V2, because you're right that
it's the sane thing to do and actually it wasn't too much work. I (and
others) do experience crashes if pll-mipi is driven below the 500 MHz
mark, so let's fix this once and for all.

> Anyway, do you know where are all those old values come from?

I've done some digging on lore and the values were originally submitted
by Icenowy Zheng as part of a series to support the pinephone's LCD [1].
There has been some refactoring after this initial submission and Ondrej
Jirman took over. But the values are still the ones submitted by
Icenowy, so I've added her to CC. I couldn't find any documentation for
this specific panel.

> And how did
> you come up with new ones?

Trial and no error. :)

No, really, it was just a lucky guess. I know nothing about LCD panels,
so I only looked at the original values:
.htotal =  720 + 40 + 40 + 40,
.vtotal = 1440 + 18 + 10 + 17,

I thought, what if every time I increase a horizontal value by 2, I
increase a vertical value by 1 (very roughly).

So I ended up with:
.htotal =  720 + 65 + 65 + 65,
.vtotal = 1440 + 30 + 22 + 29,

So, in conclusion, I've increased each of the horizontal values by 25
and each of the vertical values by 12. Then I just tried out these new
values, and the world didn't end. :)

If this is stupid, please somebody let me know.

I (and at least one postmarket OS tester) have been daily driving the
panel with these values for about a week now.

I've checked the panel's refresh rate with the following test setup:
 - I created a 60 fps video that shows the current frame number in each
   frame. The video is 10 seconds (600 frames) long. [2]
 - I played that video on my pinephone using vlc. [3]
 - I recorded the playback with a Google Pixel 5 phone at 1/8 slow
   motion (240 fps).
 - I converted the video into individual pictures [4], resized
   the pictures to 10% [5], and finally - after deleting some superfluous
   pictures at the beginning and end - I created one big collage out of
   these [6].

I've uploaded the video[7], resulting collage [8] and the individual
pictures [9].

In the resulting picture you can see that in the beginning frame 2 is
missing and frame 136 is only barely visible because it is stuck too
long on frame 135. Other than that, I think this looks pretty good.

> I guess you can't just simply change timings,
> there are probably some HW limitations? Do you know if BSP kernel support
> this panel and how this situation is solved there?

I'm not aware of any BSP kernel that supports this kernel.

>> If we only changed the constraints on the PLL_MIPI without changing the
>> panel mode, we end up with a mismatch. This, in turn, would result in
>> dropped frames, right?
>
> From what I read, I think frame rate would be higher than 60 fps. What
> exactly would happen depends on the panel.

To give this a fair comparison, I tested with the original timings but
with correcting the panel's clock rate of 74844 kHZ instead of 69000 kHz
as discussed elsewhere in this thread [10] and pll-mipi running at
500MHz (because that's really a must to run the pinephone in a stable
manner). I used the same procedure as described above. Again, I've
uploaded the resulting video [11], collage [12] and the individual files
[13].

Here, the being stuck happens much more often, e.g. frames 23, 31, 40,
49, 58, 66 etc.

So, I think, in order to have a better user experience, I think it's a
good idea to update the XBD599 panel with the new values I proposed in
this patch.

Best regards,
  Frank

[1]: https://lore.kernel.org/all/20200311162936.221613-1-icenowy@aosc.io/
[2]: ffmpeg -f lavfi -i testsrc=duration=10:size=80x50:rate=60 -vf \
   "drawtext=text=%{n}:fontsize=36:r=60:x=(w-tw)/2: y=h-(1*lh):fontcolor=white:box=1:boxcolor=0x00000099"\
   test_80x50.mp4
[3]: cvlc test_80x50.mp4  --fullscreen --play-and-exit
[4]: ffmpeg -i video_from_pixel_phone.mp4 -vsync vfr output_%04d.jpg
[5]: mogrify -resize 10% output_*.jpg
[6]: montage output_*.jpg -tile 20x -geometry +0+0 \
      verify_panel_65_65_65_30_22_29_83502.jpg
[7]: https://share.mailbox.org/ajax/share/0a471a7205211949ad7067d521194571984a15d1613d74be/1/8/Njg/NjgvMzE
[8]: https://share.mailbox.org/ajax/share/0741f90808f2df4e7d1e5078f2df43cfae732189f27e75e3/1/8/Njg/NjgvMzI
[9]: https://share.mailbox.org/ajax/share/0471bc0706bfee4e4e1a0086bfee40ecba2123a14c9b8d4d/1/8/Njg/NjgvMzA
[10]: https://lore.kernel.org/all/87v88qk3ge.fsf@oltmanns.dev/
[11]: https://share.mailbox.org/ajax/share/036036d00eac574e3f02adfeac5741dda88105026a1221f4/1/8/Njg/NjgvMzQ
[12]: https://share.mailbox.org/ajax/share/05f6d3e905e30058566cfe65e300486aa936122f2414639a/1/8/Njg/NjgvMzU
[13]: https://share.mailbox.org/ajax/share/0cf25a810cce2357c62468ecce234681a4e8e674d38d02cd/1/8/Njg/NjgvMzM

>
> Best regards,
> Jernej
>
>>
>> Best regards,
>>   Frank
>>
>> [1] Source:
>> https://elixir.bootlin.com/linux/v6.6.7/source/drivers/gpu/drm/sun4i/sun4i_tcon.c#L346
>>
>> >
>> > Best regards,
>> > Jernej
>> >
>> >> ---
>> >>  drivers/gpu/drm/panel/panel-sitronix-st7703.c | 14 +++++++-------
>> >>  1 file changed, 7 insertions(+), 7 deletions(-)
>> >>
>> >> diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7703.c b/drivers/gpu/drm/panel/panel-sitronix-st7703.c
>> >> index b55bafd1a8be..6886fd7f765e 100644
>> >> --- a/drivers/gpu/drm/panel/panel-sitronix-st7703.c
>> >> +++ b/drivers/gpu/drm/panel/panel-sitronix-st7703.c
>> >> @@ -320,14 +320,14 @@ static int xbd599_init_sequence(struct st7703 *ctx)
>> >>
>> >>  static const struct drm_display_mode xbd599_mode = {
>> >>  	.hdisplay    = 720,
>> >> -	.hsync_start = 720 + 40,
>> >> -	.hsync_end   = 720 + 40 + 40,
>> >> -	.htotal	     = 720 + 40 + 40 + 40,
>> >> +	.hsync_start = 720 + 65,
>> >> +	.hsync_end   = 720 + 65 + 65,
>> >> +	.htotal      = 720 + 65 + 65 + 65,
>> >>  	.vdisplay    = 1440,
>> >> -	.vsync_start = 1440 + 18,
>> >> -	.vsync_end   = 1440 + 18 + 10,
>> >> -	.vtotal	     = 1440 + 18 + 10 + 17,
>> >> -	.clock	     = 69000,
>> >> +	.vsync_start = 1440 + 30,
>> >> +	.vsync_end   = 1440 + 30 + 22,
>> >> +	.vtotal	     = 1440 + 30 + 22 + 29,
>> >> +	.clock	     = (720 + 65 + 65 + 65) * (1440 + 30 + 22 + 29) * 60 / 1000,
>> >>  	.flags	     = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
>> >>  	.width_mm    = 68,
>> >>  	.height_mm   = 136,
>> >>
>> >>
>>