From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 7CCCACFD342 for ; Fri, 11 Oct 2024 11:12:49 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=u1W7YZRetrR1geL14ujwVDdDkYBScmEJKU90VEkilhs=; b=SyJRx0cz9EFMpK3oM60ZssDSRP /pIWGp7Y/zKCyhFwW0bsrsPOWegjx4HiEi4l0j7qlVfnbhZYxP7FAh0Hc5VUTcJC66M3CQBXhArJT 5uulcZ7dlJ/MtWRIjcx2DUztSBQ1fFdGqDgQrymoqTBxAY1ij6Zk26rEkWOUYdMc2atwK70btLGUY 3jYW0KBXdxS99zTa+36utpqEc6kOpgYfAsF/41CG5z+sYF7Bz+p/3XZSGuSVENMTue5IQ6mU3ztYU wqrp4GA/NR9ITflaa3FDsGP33DjMxRUZsKwkfe8nZhreWHnP4IiqjIL4f25vK8klXo5jaUwjUte8C th0DyBQw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98 #2 (Red Hat Linux)) id 1szDZY-0000000G7IU-2NNt; Fri, 11 Oct 2024 11:12:40 +0000 Received: from nyc.source.kernel.org ([147.75.193.91]) by bombadil.infradead.org with esmtps (Exim 4.98 #2 (Red Hat Linux)) id 1szDTC-0000000G65y-1G75 for linux-arm-kernel@lists.infradead.org; Fri, 11 Oct 2024 11:06:07 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by nyc.source.kernel.org (Postfix) with ESMTP id 2EDE5A436A3; Fri, 11 Oct 2024 11:05:56 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 161C6C4CEC3; Fri, 11 Oct 2024 11:06:04 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1728644764; bh=+afMQpX0JvOchhAJniw8uaMvP8EFyAlup01m6OURoHI=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=X4OulmIhynqoIG6cwL/oDYypb1Cra/XDVG+ommmLbQdd01N081OD7DEn2cJTN38Q6 vV0ypSvGHzSufSE0IfmSmLfO1Zh0quEYL3s9tE8ytn0H+tVzkMVvaMXo+HE0Ftv4GR fOAea25nsJcs8+hDeTZxyqO7SRZzOildiPxtaunCtqZrCIoSQfpSqhcI/8JKvVqeEl UulNNoD9fxKF3H9iEDegVSYgkZSz0vf5hj/mKPCegzGIgceyqsYaWH77Bm3OWNPMzW E2GhutJD20SAHv7tG85QM1KcaLx6GweMA05hawDIxPKPj8LSP1HjCPe68etrZClmGD xO9oZH3Tyi15A== Date: Fri, 11 Oct 2024 13:06:01 +0200 From: Maxime Ripard To: Liu Ying Cc: dri-devel@lists.freedesktop.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, imx@lists.linux.dev, linux-arm-kernel@lists.infradead.org, andrzej.hajda@intel.com, neil.armstrong@linaro.org, rfoss@kernel.org, Laurent.pinchart@ideasonboard.com, jonas@kwiboo.se, jernej.skrabec@gmail.com, maarten.lankhorst@linux.intel.com, tzimmermann@suse.de, airlied@gmail.com, simona@ffwll.ch, robh@kernel.org, krzk+dt@kernel.org, conor+dt@kernel.org, shawnguo@kernel.org, s.hauer@pengutronix.de, kernel@pengutronix.de, festevam@gmail.com, catalin.marinas@arm.com, will@kernel.org, quic_bjorande@quicinc.com, geert+renesas@glider.be, dmitry.baryshkov@linaro.org, arnd@arndb.de, nfraprado@collabora.com, o.rempel@pengutronix.de, y.moog@phytec.de Subject: Re: [PATCH 4/8] drm/bridge: fsl-ldb: Use clk_round_rate() to validate "ldb" clock rate Message-ID: <20241011-mottled-translucent-dodo-8877e6@houat> References: <20240930052903.168881-1-victor.liu@nxp.com> <20240930052903.168881-5-victor.liu@nxp.com> <2on4bu5jsxvaxckqz3wouwrf2z6nwbtv34ek4xda2dvobqhbsf@g7z7kxq5xrxi> <5fb80bf6-96be-4654-bd54-dc4f1d5136ae@nxp.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha384; protocol="application/pgp-signature"; boundary="x757sqlahb6r4ntt" Content-Disposition: inline In-Reply-To: <5fb80bf6-96be-4654-bd54-dc4f1d5136ae@nxp.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20241011_040606_498370_1E8CC268 X-CRM114-Status: GOOD ( 35.29 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org --x757sqlahb6r4ntt Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Sep 30, 2024 at 03:55:30PM GMT, Liu Ying wrote: > On 09/30/2024, Maxime Ripard wrote: > > On Mon, Sep 30, 2024 at 01:28:59PM GMT, Liu Ying wrote: > >> Multiple display modes could be read from a display device's EDID. > >> Use clk_round_rate() to validate the "ldb" clock rate for each mode > >> in drm_bridge_funcs::mode_valid() to filter unsupported modes out. > >> > >> Also, if the "ldb" clock and the pixel clock are sibling in clock > >> tree, use clk_round_rate() to validate the pixel clock rate against > >> the "ldb" clock. This is not done in display controller driver > >> because drm_crtc_helper_funcs::mode_valid() may not decide to do > >> the validation or not if multiple encoders are connected to the CRTC, > >> e.g., i.MX93 LCDIF may connect with MIPI DSI controller, LDB and > >> parallel display output simultaneously. > >> > >> Signed-off-by: Liu Ying > >> --- > >> drivers/gpu/drm/bridge/fsl-ldb.c | 22 ++++++++++++++++++++++ > >> 1 file changed, 22 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c b/drivers/gpu/drm/bridge= /fsl-ldb.c > >> index b559f3e0bef6..ee8471c86617 100644 > >> --- a/drivers/gpu/drm/bridge/fsl-ldb.c > >> +++ b/drivers/gpu/drm/bridge/fsl-ldb.c > >> @@ -11,6 +11,7 @@ > >> #include > >> #include > >> #include > >> +#include > >> =20 > >> #include > >> #include > >> @@ -64,6 +65,7 @@ struct fsl_ldb_devdata { > >> u32 lvds_ctrl; > >> bool lvds_en_bit; > >> bool single_ctrl_reg; > >> + bool ldb_clk_pixel_clk_sibling; > >> }; > >> =20 > >> static const struct fsl_ldb_devdata fsl_ldb_devdata[] =3D { > >> @@ -74,11 +76,13 @@ static const struct fsl_ldb_devdata fsl_ldb_devdat= a[] =3D { > >> [IMX8MP_LDB] =3D { > >> .ldb_ctrl =3D 0x5c, > >> .lvds_ctrl =3D 0x128, > >> + .ldb_clk_pixel_clk_sibling =3D true, > >> }, > >> [IMX93_LDB] =3D { > >> .ldb_ctrl =3D 0x20, > >> .lvds_ctrl =3D 0x24, > >> .lvds_en_bit =3D true, > >> + .ldb_clk_pixel_clk_sibling =3D true, > >> }, > >> }; > >> =20 > >> @@ -269,11 +273,29 @@ fsl_ldb_mode_valid(struct drm_bridge *bridge, > >> const struct drm_display_info *info, > >> const struct drm_display_mode *mode) > >> { > >> + unsigned long link_freq, pclk_rate, rounded_pclk_rate; > >> struct fsl_ldb *fsl_ldb =3D to_fsl_ldb(bridge); > >> =20 > >> if (mode->clock > (fsl_ldb_is_dual(fsl_ldb) ? 160000 : 80000)) > >> return MODE_CLOCK_HIGH; > >> =20 > >> + /* Validate "ldb" clock rate. */ > >> + link_freq =3D fsl_ldb_link_frequency(fsl_ldb, mode->clock); > >> + if (link_freq !=3D clk_round_rate(fsl_ldb->clk, link_freq)) > >> + return MODE_NOCLOCK; > >> + > >> + /* > >> + * Use "ldb" clock to validate pixel clock rate, > >> + * if the two clocks are sibling. > >> + */ > >> + if (fsl_ldb->devdata->ldb_clk_pixel_clk_sibling) { > >> + pclk_rate =3D mode->clock * HZ_PER_KHZ; > >> + > >> + rounded_pclk_rate =3D clk_round_rate(fsl_ldb->clk, pclk_rate); > >> + if (rounded_pclk_rate !=3D pclk_rate) > >> + return MODE_NOCLOCK; > >> + } > >> + > >=20 > > I guess this is to workaround the fact that the parent rate would be > > changed, and thus the sibling rate as well? This should be documented in > > a comment if so. >=20 > This is to workaround the fact that the display controller driver > (lcdif_kms.c) cannot do the mode validation against pixel clock, as > the commit message mentions. That part is still not super clear to me, but it's also not super important to the discussion. My point is: from a clock API standpoint, there's absolutely no reason to consider sibling clocks. clk_round_rate() should give you the rate you want. If it affects other clocks it shouldn't, it's a clock driver bug. You might want to workaround it, but this is definitely not something you should gloss over: it's a hack, it needs to be documented as such. > The parent clock is IMX8MP_VIDEO_PLL1_OUT and it's clock rate is not > supposed to be changed any more once IMX8MP_VIDEO_PLL1 clock rate is > set by using DT assigned-clock-rates property. For i.MX8MP EVK, the > clock rate is assigned to 1039500000Hz in imx8mp.dtsi in media_blk_ctrl > node. There's two things wrong with what you just described: - assigned-clock-rates never provided the guarantee that the clock rate wouldn't change later on. So if you rely on that, here's your first bug. - If the parent clock rate must not change, why does that clock has SET_RATE_PARENT then? Because that's the bug you're trying to work around. Maxime --x757sqlahb6r4ntt Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iJUEABMJAB0WIQTkHFbLp4ejekA/qfgnX84Zoj2+dgUCZwkGmQAKCRAnX84Zoj2+ dpmzAX42+f8i9iWsZTTD5OGlkpoItRqHSN7O6d+7Lxr/b8KuqFcDNVloMtOgxtQ5 VJUOg80Bf1SKxcQSA71NDLJi1xYsqMWk43y5/K7Xr7fwOcMDdGqyODG1onnbtzsc vPqFZLtSMQ== =1Lq7 -----END PGP SIGNATURE----- --x757sqlahb6r4ntt--