From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) (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 5787218DF8B for ; Wed, 9 Oct 2024 09:55:51 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=213.167.242.64 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1728467753; cv=none; b=YJ2Q8omDI1fB+t+SRCOh8kQipa+9jGDs0lRBHR87JufI2MQbe7ryFBv/JWYbGH+21C75YFs2x6R2BZ8R91Ax2el6C8BM1J75wOFBcyJ/RU0HcG0vqsb4KIT8kK1wyo60PR8yrH59Do/RnSUcTnF9k/oyrK9PBxizu1ESMmGYVWE= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1728467753; c=relaxed/simple; bh=eq5e+ixufpNf6HI4bCWJ60TojZW503IRWi5DhqrGor0=; h=Message-ID:Subject:From:To:Cc:Date:In-Reply-To:References: Content-Type:MIME-Version; b=OPruyqGVnh6NM+aMZ3sZ3Q1J6DBjtbpV5bQ8HjkAH8KYAEnkKnr3WYMDPOhhsokg0Nwnt1GtHa/5S3/fvteqMescrYiwZnxr5uzdF88pAUDb6NZwPYePtKi1TsGY8k2pb/gSBsv7hfhPTf2NbuItaIEQjubVOMMUiL62tlL8IzE= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=ideasonboard.com; spf=pass smtp.mailfrom=ideasonboard.com; dkim=pass (1024-bit key) header.d=ideasonboard.com header.i=@ideasonboard.com header.b=b4ac6qn0; arc=none smtp.client-ip=213.167.242.64 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=ideasonboard.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=ideasonboard.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="b4ac6qn0" Received: from isaac-ThinkPad-T16-Gen-2.local (cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 57679594; Wed, 9 Oct 2024 11:54:05 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1728467646; bh=eq5e+ixufpNf6HI4bCWJ60TojZW503IRWi5DhqrGor0=; h=Subject:From:To:Cc:Date:In-Reply-To:References:From; b=b4ac6qn0PVte+Iw3UZSmOGc1rnnctfO/AksV0E/gtofia22hWkK20iG97h6O1WFSR 6GxQwQNPYurzYQdAMGACtVL05L10ai2rd9CtD40KRCCYWMRpl2+gddnmbcKCBPOTlx mBfMKhAOGQCzz0CwVLyKyZfcjfkZHEOvSWy7tPQ0= Message-ID: <50f10a422dd3a7099e2e2724f9401dbae41ea529.camel@ideasonboard.com> Subject: Re: [PATCH] drm: lcdif: Use adjusted_mode .clock instead of .crtc_clock From: Isaac Scott To: Marek Vasut , Alexander Stein , dri-devel@lists.freedesktop.org Cc: Daniel Vetter , David Airlie , Fabio Estevam , Lucas Stach , "Lukas F . Hartmann" , Maarten Lankhorst , Maxime Ripard , Pengutronix Kernel Team , Sascha Hauer , Shawn Guo , Stefan Agner , Thomas Zimmermann , imx@lists.linux.dev, kernel@dh-electronics.com, linux-arm-kernel@lists.infradead.org, kieran.bingham@ideasonboard.com Date: Wed, 09 Oct 2024 10:55:38 +0100 In-Reply-To: <84f505af-1066-4fcf-84b7-28c152c09b89@denx.de> References: <20240531202813.277109-1-marex@denx.de> <1897634.CQOukoFCf9@steina-w> <7ae0cd7774f4b3e30cc033a7e543546732dbced0.camel@ideasonboard.com> <64e18ceed5279a9346a6a1141f02ead93383bd1e.camel@ideasonboard.com> <84f505af-1066-4fcf-84b7-28c152c09b89@denx.de> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable User-Agent: Evolution 3.54.0 (by Flathub.org) Precedence: bulk X-Mailing-List: imx@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 On Tue, 2024-10-08 at 23:48 +0200, Marek Vasut wrote: > On 10/8/24 12:07 PM, Isaac Scott wrote: > > On Mon, 2024-10-07 at 20:06 +0200, Marek Vasut wrote: > > > On 10/7/24 7:01 PM, Isaac Scott wrote: > > > > Hi Marek, > > >=20 > > > Hi, > > >=20 > > > > On Sat, 2024-07-06 at 02:16 +0200, Marek Vasut wrote: > > > > > On 6/24/24 11:19 AM, Alexander Stein wrote: > > > > > > Am Freitag, 31. Mai 2024, 22:27:21 CEST schrieb Marek > > > > > > Vasut: > > > > > > > In case an upstream bridge modified the required clock > > > > > > > frequency > > > > > > > in its .atomic_check callback by setting > > > > > > > adjusted_mode.clock > > > > > > > , > > > > > > > make sure that clock frequency is generated by the > > > > > > > LCDIFv3 > > > > > > > block. > > > > > > >=20 > > > > > > > This is useful e.g. when LCDIFv3 feeds DSIM which feeds > > > > > > > TC358767 > > > > > > > with (e)DP output, where the TC358767 expects precise > > > > > > > timing > > > > > > > on > > > > > > > its input side, the precise timing must be generated by > > > > > > > the > > > > > > > LCDIF. > > > > > > >=20 > > > > > > > Signed-off-by: Marek Vasut > > > > > >=20 > > > > > > With the other rc358767 patches in place, this does the > > > > > > trick. > > > > > > Reviewed-by: Alexander Stein > > > > > > > > > > >=20 > > > > > I'll pick this up next week if there is no objection. > > > >=20 > > > > Unfortunately, this has caused a regression that is present in > > > > v6.12- > > > > rc1 on the i.MX8MP PHYTEC Pollux using the > > > > arch/arm64/boot/dts/freescale/imx8mp-phyboard-pollux-rdk.dts. > > > > The > > > > display is the edt,etml1010g3dra panel, as per the upstream > > > > dts. We > > > > bisected to this commit, and reverting this change fixed the > > > > screen. > > > >=20 > > > > We then tried to retest this on top of v6.12-rc2, and found we > > > > also > > > > had > > > > to revert commit ff06ea04e4cf3ba2f025024776e83bfbdfa05155 > > > > ("clk: > > > > imx: > > > > clk-imx8mp: Allow media_disp pixel clock reconfigure parent > > > > rate") > > > > alongside this. Reverting these two commits makes the display > > > > work > > > > again at -rc2. > > > >=20 > > > > Do you have any suggestions on anything we might be missing on > > > > our > > > > end? > > > > Please let me know if there's anything you'd like me to test as > > > > I'm > > > > not > > > > sure what the underlying fault was here. > > > I believe what is going on is that the LCDIF cannot configure its > > > upstream clock because something else is already using those > > > clock > > > and > > > it set those clock to a specific frequency. LCDIF is now trying > > > to > > > configure those clock to match the LVDS panel, and it fails, so > > > it > > > tries > > > to set some approximate clock and that is not good enough for the > > > LVDS > > > panel. > > >=20 > > > Can you share dump of /sys/kernel/debug/clk/clk_summary on > > > failing > > > and > > > working system ? You might see the difference around the "video" > > > clock. > > >=20 > > > (I have seen this behavior before, the fix was usually a matter > > > of > > > moving one of the LCDIFs to another upstream clock like PLL3, so > > > it > > > can > > > pick well matching output clock instead of some horrid > > > approximation > > > which then drives the panel likely out of specification) > >=20 > > Hi Marek, > >=20 > > Please find attached the clk_summary for v6.12-rc2 before and after > > the > > reversion (the one after the reversion is 6.12- > > rc2_summary_postfix). > Thank you, this helped greatly. >=20 > I believe I know why it used to kind-of work for you, but I'm afraid=20 > this used to work by sheer chance and it does not really work > correctly=20 > for the panel you use, even if the panel likely does show the correct > content. But, there is a way to make it work properly for the panel > you use. >=20 > First of all, the pixel clock never really matched the panel-simple.c > pixel clock for the edt_etml1010g3dra_timing: >=20 > $ grep '\' 6.12-rc2_summary_postfix > =C2=A0=C2=A0 media_disp2_pix=C2=A0 1=C2=A0 1=C2=A0 0=C2=A0 74250000 ... > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0 ^^^^^^^^ >=20 > $ grep -A 1 edt_etml1010g3dra_timing drivers/gpu/drm/panel/panel- > simple.c > static const struct display_timing edt_etml1010g3dra_timing =3D { > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 .pixelclock =3D { 663000= 00, 72400000, 78900000 }, > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ^^^^^^^^ >=20 > The pixel clock are within tolerance, but there is a discrepancy=20 > 74250000 !=3D 72400000 . >=20 > Since commit 94e6197dadc9 ("arm64: dts: imx8mp: Add LCDIF2 & LDB > nodes")=20 > the IMX8MP_VIDEO_PLL1_OUT is set to a very specific frequency of=20 > 1039500000 Hz, which tidily divides by 2 to 519750000 Hz (which is > your=20 > LVDS serializer frequency) and divides by 7 to 74250000 Hz which is > your=20 > LCDIF pixel clock. >=20 > This Video PLL1 configuration since moved to &media_blk_ctrl {} , but > it=20 > is still in the imx8mp.dtsi . Therefore, to make your panel work at > the=20 > correct desired pixel clock frequency instead of some random one=20 > inherited from imx8mp.dtsi, add the following to the pollux DT, I=20 > believe that will fix the problem and is the correct fix: >=20 > &media_blk_ctrl { > =C2=A0=C2=A0=C2=A0 // 506800000 =3D 72400000 * 7 (for single-link LVDS, t= his is > enough) > =C2=A0=C2=A0=C2=A0 // there is no need to multiply the clock by * 2 > =C2=A0=C2=A0=C2=A0 assigned-clock-rates =3D <500000000>, <200000000>, <0>= , <0>,=20 > <500000000>, <506800000>; > }; >=20 > Can you please test whether this works and the pixel clock are > accurate=20 > in /sys/kernel/debug/clk/clk_summary ? Interestingly, after making the change you suggested to=C2=A0imx8mp- phyboard-pollux-rdk.dts before the two reversions, the display now seems to work. Please see below for the relevant section of the new clk_summary referring to media_disp2_pix: video_pll1_ref_sel 1 1 0 24000000 =20 0 0 50000 Y deviceless =20 no_connection_id=20 video_pll1 1 1 0 =20 506800000 0 0 50000 Y deviceless =20 no_connection_ video_pll1_bypass 1 1 0 =20 506800000 0 0 50000 Y deviceless =20 no_connecti video_pll1_out 2 2 0 =20 506800000 0 0 50000 Y deviceless =20 no_conne media_ldb 1 1 0 =20 506800000 0 0 50000 Y deviceless=20 no_co media_ldb_root_clk 1 1 0 =20 506800000 0 0 50000 Y =20 32ec0000.blk-ctrl:bridge@5c l =20 deviceless no media_disp2_pix 1 1 0 72400000 0 0 50000 Y deviceless =20 no_co media_disp2_pix_root_clk 1 1 0 =20 72400000 0 0 50000 Y =20 32e90000.display-controller =20 32ec0000.blk-ctrl di =20 deviceless no media_disp1_pix 0 0 0 =20 506800000 0 0 50000 N deviceless=20 no_co media_disp1_pix_root_clk 0 0 0 =20 506800000 0 0 50000 N =20 32ec0000.blk-ctrl =20 =20 deviceless no media_mipi_phy1_ref 0 0 0 23036364 0 0 50000 N deviceless =20 no_co media_mipi_phy1_ref_root 0 0 0 =20 23036364 0 0 50000 N =20 32ec0000.blk-ctrl =20 The media_disp2_pix clock now seems to be correct at 724000000 after your changes.