Message ID | 20231218-pinephone-pll-fixes-v1-5-e238b6ed6dc1@oltmanns.dev |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel+bounces-3752-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:7300:24d3:b0:fb:cd0c:d3e with SMTP id r19csp1246153dyi; Mon, 18 Dec 2023 05:40:57 -0800 (PST) X-Google-Smtp-Source: AGHT+IEmTfQDJgKYHkBHFK7EHsPSyzEo2H7W6J33Eujrx4O9P0bq+xkd1Zo7kblWD5+9/AkQZuAa X-Received: by 2002:a05:6e02:318f:b0:35f:a535:4da3 with SMTP id cb15-20020a056e02318f00b0035fa5354da3mr3067551ilb.130.1702906857230; Mon, 18 Dec 2023 05:40:57 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1702906857; cv=none; d=google.com; s=arc-20160816; b=vnaWXcrefM4pRy0XIzdZ8QDZNQlfSda4UVvQXJz2TsrmCaydk/CXQEUiVvVE0VAB+b lrMdLV5cOUOQem71H8aaaRE1PYRvcsMYN6wS7m0W2MrGCivsu380G9kQNJ2lA1OzVc3x bVFUYtu6prpfSEONnLiUbIGoEgfn6NG6vhLDpxocA7Fzn3TdI2BB7pPwGGyvJIc+qaWJ ZbUd4S8x41na2PHUuB4d4psoXflA4nfnWB4yAiGFUW2Fa/vLUQEXT8aJeDwqHJkwhJuJ Eh0amQCXbp4v407UW2vOgMSkzfEvI31iEjUn6HwHSOddZuzZPlBcjJozmDRxBN4O31mI 2SGQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=cc:to:in-reply-to:references:message-id:content-transfer-encoding :mime-version:list-unsubscribe:list-subscribe:list-id:precedence :subject:date:from:dkim-signature; bh=zHjXX8T22A8vSDhmcbXCgVqx2gpy6+LirkB+PVRAonw=; fh=Cik4nBs1VMxe7SDk4FjLPu75elh3pWslp9/ZGKW1BWs=; b=dDyUcqGxj2k5Onf4ivJVEVJ5nIsX+P3ZBO1jH3u8WdTCydnyMOxPLiVe7N9mmUQBSp WmAa2CYd6fw8qPO7A67qRu1k/u2z4nlb2/3HjCJPa+4KT812BjzoKkBETQBzSJIgZrXN 8m99599BjmnoN4Rj3RYRcnTkQNjxNm4MBsEmoMpU88xW9BqW5It72aE7noLgXykhEp/u tEI1UrXarch4TTL3E+k57BUoBN4af0A6NK8ZFTJj9PMRzy+KmpwnBVNdwOACnswZwwIf aFuesFhANr7V4TBkq/x5jnPE46lkze9MKb+9P+W++aXsvjMRDJb4xYMzUG6kraI1ioVu Nbqg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@oltmanns.dev header.s=MBO0001 header.b=eLflKYcE; spf=pass (google.com: domain of linux-kernel+bounces-3752-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) smtp.mailfrom="linux-kernel+bounces-3752-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=oltmanns.dev Received: from sy.mirrors.kernel.org (sy.mirrors.kernel.org. [147.75.48.161]) by mx.google.com with ESMTPS id a22-20020a631a56000000b005be1ee5b99fsi17581129pgm.526.2023.12.18.05.40.56 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 18 Dec 2023 05:40:57 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-3752-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161; Authentication-Results: mx.google.com; dkim=pass header.i=@oltmanns.dev header.s=MBO0001 header.b=eLflKYcE; spf=pass (google.com: domain of linux-kernel+bounces-3752-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) smtp.mailfrom="linux-kernel+bounces-3752-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=oltmanns.dev Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by sy.mirrors.kernel.org (Postfix) with ESMTPS id AFD89B22E69 for <ouuuleilei@gmail.com>; Mon, 18 Dec 2023 13:37:06 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 7D9423786A; Mon, 18 Dec 2023 13:36:27 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=oltmanns.dev header.i=@oltmanns.dev header.b="eLflKYcE" X-Original-To: linux-kernel@vger.kernel.org Received: from mout-p-201.mailbox.org (mout-p-201.mailbox.org [80.241.56.171]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 9E84D1D15C; Mon, 18 Dec 2023 13:36:21 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=oltmanns.dev Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=oltmanns.dev Received: from smtp1.mailbox.org (smtp1.mailbox.org [10.196.197.1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by mout-p-201.mailbox.org (Postfix) with ESMTPS id 4Sv17Q1Z1Cz9sv0; Mon, 18 Dec 2023 14:36:18 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oltmanns.dev; s=MBO0001; t=1702906578; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=zHjXX8T22A8vSDhmcbXCgVqx2gpy6+LirkB+PVRAonw=; b=eLflKYcE7WuO2izSmh+X4mn7M4W4r5tiLf9tLy/gpwJ+HYuWpLQlyVLDfp3gMot+v2Cv2N EiK/PmJIEErOafvsXPHmGQY90NZMbO/zi1o7/xuzR2ocO/zYJ/N6j/s/YVYfb39sT3SwtT aoww2i0Hke2YsfumQiIWt5egYYkJCVFq9CmRzwfLV3N+waySq5cjnUT1xoiZ3BSY9ChWB3 U7I7zD9g71JhPSGIUVT448PdHgMiaNjileX7O6INdR6Jrij23kJ3GNRPPEaVbmlIW+BcsX 1DwPFtGSrXbqD8QVz8/tksdd7MGBqGQ42fZMYiESibwLRyicfQwoUSb2sybBmQ== From: Frank Oltmanns <frank@oltmanns.dev> Date: Mon, 18 Dec 2023 14:35:23 +0100 Subject: [PATCH 5/5] drm/panel: st7703: Drive XBD599 panel at higher clock rate Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: <linux-kernel.vger.kernel.org> List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <20231218-pinephone-pll-fixes-v1-5-e238b6ed6dc1@oltmanns.dev> References: <20231218-pinephone-pll-fixes-v1-0-e238b6ed6dc1@oltmanns.dev> In-Reply-To: <20231218-pinephone-pll-fixes-v1-0-e238b6ed6dc1@oltmanns.dev> To: Michael Turquette <mturquette@baylibre.com>, Stephen Boyd <sboyd@kernel.org>, Chen-Yu Tsai <wens@csie.org>, Jernej Skrabec <jernej.skrabec@gmail.com>, Samuel Holland <samuel@sholland.org>, =?utf-8?q?Guido_G=C3=BCnther?= <agx@sigxcpu.org>, Purism Kernel Team <kernel@puri.sm>, Ondrej Jirman <megi@xff.cz>, Neil Armstrong <neil.armstrong@linaro.org>, Jessica Zhang <quic_jesszhan@quicinc.com>, Sam Ravnborg <sam@ravnborg.org>, Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, Maxime Ripard <mripard@kernel.org>, Thomas Zimmermann <tzimmermann@suse.de>, David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch> Cc: linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-sunxi@lists.linux.dev, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, Frank Oltmanns <frank@oltmanns.dev> X-Developer-Signature: v=1; a=openpgp-sha256; l=1524; i=frank@oltmanns.dev; h=from:subject:message-id; bh=SAMO997ikQjd5G5gtpw1xcptKu2mOxUiT+50d/YBtmI=; b=owEB7QES/pANAwAIAZppogiUStPHAcsmYgBlgEq1fGk87SLdf4iGeylAn1TQ+0H3qF+BNVT6u HjpEFxEqgCJAbMEAAEIAB0WIQQC/SV7f5DmuaVET5aaaaIIlErTxwUCZYBKtQAKCRCaaaIIlErT x9WPC/97CEvcAl7/lqqTgFUkj4zb5ppCtk9rJIICkdmTftzH99U8PSxmXE0nEf8iTgYdjng9Bx0 k8h58NQZlu9Ps/sbc+lo0PbQuWCGnDUYRSw3H97Ct1ju7uKs714t69LhGUtgBK1JFsg4vrhv9Ch d9E6vrurGJ+KKUbl1lHArTu3cD9ygcg1q6K1kfD1AsHzfH/dQWXUYjSTsrv8jgagl7VerjnL4r6 dqLNaVl1q5iSJV/RqerAulhUjp6A0frnJOsnltaZiECVdfsoa2ermOBOgrApTJHE8wsJwTw0FXy pkBvnm9ZXsaQNI+jbMt5sWUZDld7hwsIn15rF6BpanhgXL0XS8fDVgpKQFLfLK6+HyfkX8js4u5 X9xCCISayV/Nd+L7Wlx0um2UprwxuUoJ5uwkLqD9urRVV6ievIWaBnoNMSnOoQs5Xk8GancYKVF Tun737PSoCemEJMoweiWRqO/Jb7lnbvPtEmDmapfv8mKeBY6rfIUXBPJc2BlVRJ+rq9Gs= X-Developer-Key: i=frank@oltmanns.dev; a=openpgp; fpr=02FD257B7F90E6B9A5444F969A69A208944AD3C7 X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1785627260895198912 X-GMAIL-MSGID: 1785627260895198912 |
Series |
Pinephone video out fixes (flipping between two frames)
|
|
Commit Message
Frank Oltmanns
Dec. 18, 2023, 1:35 p.m. UTC
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>
---
drivers/gpu/drm/panel/panel-sitronix-st7703.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
Comments
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, >> >>
Ok, I've done more detailed testing, and it seems this patch results in lots of dropped frames. I'm sorry for not being more thorough earlier. I'll do some more testing without this patch and might have to either remove it from V2 of this series. I need to see if the same stability can be achieved when running PLL-MIPI outside its specied range. Best regards, Frank 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. > > Anyway, do you know where are all those old values come from? And how did > you come up with new ones? 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? > >> >> 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. > > 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, >> >> >> >> >>
On 2023-12-20 at 19:57:06 +0100, Frank Oltmanns <frank@oltmanns.dev> wrote: > Ok, I've done more detailed testing, and it seems this patch results in > lots of dropped frames. I'm sorry for not being more thorough earlier. > I'll do some more testing without this patch and might have to either > remove it from V2 of this series. > > I need to see if the same stability can be achieved when running > PLL-MIPI outside its specied range. I've done some more (load) testing and observing the panel for dropped frames. The conclusion I draw from those results is that this patch isn't necessary for the pinephone. It would be enough to use the correct clock rate based on the existing values [*]: - .clock = 69000, + .clock = (720 + 40 + 40 + 40) * (1440 + 18 + 10 + 17) * 60 / 1000, I've asked in the postmarketOS community for a bit more testing. They already have a merge request that contains these changes [2]. This means that we would continue to drive PLL-MIPI outside it's specified range. I have, so far, not experienced any downside of doing so. It seems enough to fix the ratios that are part of the first four patches in this series without introducing a min and max rate. In conclusion, I'll soon (after some more feedback from the fine folks at postmarketOS) submit a V2 that addresses the fixes requested in the first four patches of this series. I'll drop the existing PATCH 5 and replace it with the one I sent in February [1] instead. After that, just for fun, I'll probably look into min_rate and max_rate for nkm clocks and which consequences it has on the pinephone. I might or might not send a follow up series for that. However, if the pinephone runs stable without it, it's not a high priority for me. Best regards, Frank [*] I've already submitted a patch in February '23 [1]. It was of little use back then because the A64's PLL-MIPI clock was not able to run close to that rate. But since kernel 6.6 PLL-MIPI is able to set it's parent rate, so that it can come quite close to the required rate: + Panel requires 74.844 MHz with the current timings. +-> tcon-data-clock rate should be 112.266 MHz (panel*24/4/4). +-> PLL-MIPI rate should be 449.064 MHz (TCON0 * 4) The 6.6 kernel the following rates are possible: + PLL-MIPI: ~448.984615 MHz +-> tcon-data-clock: ~112.246153 +-> panel: ~74.830768 MHz Which leaves us with a vertical refresh rate of ~59.989 Hz, deviating less then 0.2% from the ideal 60Hz. That's probably closer than the accumulated accuracy of all involved components can reliably achieve. I'd say, let's leave it at that. [1]: https://lore.kernel.org/lkml/20230219114553.288057-2-frank@oltmanns.dev/ [2]: https://gitlab.com/postmarketOS/pmaports/-/merge_requests/4645 > > Best regards, > Frank > > 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. >> >> Anyway, do you know where are all those old values come from? And how did >> you come up with new ones? 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? >> >>> >>> 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. >> >> 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, >>> >> >>> >> >>>
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, >> >> >> >> >>
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,