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

From: Frank Oltmanns <frank@oltmanns.dev>
To: "Jernej Škrabec" <jernej.skrabec@gmail.com>
Cc: "Neil Armstrong" <neil.armstrong@linaro.org>,
	"Purism Kernel Team" <kernel@puri.sm>,
	"Samuel Holland" <samuel@sholland.org>,
	"Stephen Boyd" <sboyd@kernel.org>,
	"David Airlie" <airlied@gmail.com>,
	"Michael Turquette" <mturquette@baylibre.com>,
	linux-kernel@vger.kernel.org,
	"Maxime Ripard" <mripard@kernel.org>,
	linux-clk@vger.kernel.org, linux-sunxi@lists.linux.dev,
	"Chen-Yu Tsai" <wens@csie.org>, "Ondrej Jirman" <megi@xff.cz>,
	dri-devel@lists.freedesktop.org,
	"Thomas Zimmermann" <tzimmermann@suse.de>,
	"Jessica Zhang" <quic_jesszhan@quicinc.com>,
	"Sam Ravnborg" <sam@ravnborg.org>,
	"Guido Günther" <agx@sigxcpu.org>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 5/5] drm/panel: st7703: Drive XBD599 panel at higher clock rate
Date: Wed, 20 Dec 2023 08:14:27 +0100	[thread overview]
Message-ID: <87edfh9ud8.fsf@oltmanns.dev> (raw)
In-Reply-To: <10386431.nUPlyArG6x@jernej-laptop>

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.

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?

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