From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtpout-04.galae.net (smtpout-04.galae.net [185.171.202.116]) (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 F3BC4344DB4 for ; Tue, 3 Feb 2026 12:42:19 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=185.171.202.116 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1770122543; cv=none; b=C4WsKkGlI2urcC3sajP6bNg3dbKulBSs/TJocLeSSsTY+6xk5H5DI5l9V4twbfN6ns0Jb9ySrElxUhWpG0wsBXdwLq5IJIoukv5ZzxI/Bn1p6NTpaEbRN2U8NTEq0fQK5l840ysoLgfU/ClaLMJOU/dvYzovkJaPq8hcJFOyOmA= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1770122543; c=relaxed/simple; bh=MpBOmlNnltjv4jxdfc8DVnH3gvt4Oi8mLikRQjqsK6M=; h=Mime-Version:Content-Type:Date:Message-Id:To:From:Subject:Cc: References:In-Reply-To; b=cg6wUobvdQK93GaUR5TMy1WfnhtGD06sAV6S49QS592npicVNJ30rcfRNEgKx2N/cRB9H7rzR0rPY9E9h2yAis+9h7lKRYcE1A8LFU7Tn8nWxUYo8ebFtpe22mCkHqbOaGAtSU0ZSAsSI+Xwt5AbevBs/gcXyQGBjqK3S2YI5lc= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=bootlin.com; spf=pass smtp.mailfrom=bootlin.com; dkim=pass (2048-bit key) header.d=bootlin.com header.i=@bootlin.com header.b=kssnCyTG; arc=none smtp.client-ip=185.171.202.116 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=bootlin.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=bootlin.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=bootlin.com header.i=@bootlin.com header.b="kssnCyTG" Received: from smtpout-01.galae.net (smtpout-01.galae.net [212.83.139.233]) by smtpout-04.galae.net (Postfix) with ESMTPS id 9FD19C211FF; Tue, 3 Feb 2026 12:42:19 +0000 (UTC) Received: from mail.galae.net (mail.galae.net [212.83.136.155]) by smtpout-01.galae.net (Postfix) with ESMTPS id 5D4AA60728; Tue, 3 Feb 2026 12:42:14 +0000 (UTC) Received: from [127.0.0.1] (localhost [127.0.0.1]) by localhost (Mailerdaemon) with ESMTPSA id 641AA119A8888; Tue, 3 Feb 2026 13:42:06 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bootlin.com; s=dkim; t=1770122533; h=from:subject:date:message-id:to:cc:mime-version:content-type: content-transfer-encoding:in-reply-to:references; bh=mjD/XeGsaFZTGLaSJ1QT/vbx0pLPYMqPljpuTVuY9Vs=; b=kssnCyTG1OTqgCpgTxqn0mZt70/JjYnao9N82p5/0ro7b5HeMHncSVb+y4JP5r+KjgK+OY mBTwqweLrzP1zwHpnsAR2c+vKHReh2BM7alEJNAUekMfitLovdIsfEGvtE5ugkw+sYghXN jfRapWPH0TuW/zkB/GZY65wxkOZiWWLobRRJQMmN2PzLZg8eCZ1Wlbmr3KrxX3E49XRKie 0NAhrgXsT7IrfeAaxxkOzS8pAf6UEUZzIU0dt89koD+tH/cHDjIHeULrOBBoH3LUQHd6NE jR+1F6LYXeoVcvzigIr0LAssOwaqyvp5r6Pzh5Q2C8DI6pgXHs9qmBPQ2irC6Q== Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Tue, 03 Feb 2026 13:42:05 +0100 Message-Id: To: "Tomi Valkeinen" , "Inki Dae" , "Jagan Teki" , "Marek Szyprowski" , "Andrzej Hajda" , "Neil Armstrong" , "Robert Foss" , "Laurent Pinchart" , "Jonas Karlman" , "Jernej Skrabec" , "Maarten Lankhorst" , "Maxime Ripard" , "Thomas Zimmermann" , "David Airlie" , "Simona Vetter" , "Aradhya Bhatia" , "Dmitry Baryshkov" From: "Luca Ceresoli" Subject: Re: [PATCH] drm/bridge: samsung-dsim: Fix init order Cc: , , "Hiago De Franco" , "Francesco Dolcini" X-Mailer: aerc 0.20.1 References: <20250619-samsung-dsim-fix-v1-1-6b5de68fb115@ideasonboard.com> In-Reply-To: <20250619-samsung-dsim-fix-v1-1-6b5de68fb115@ideasonboard.com> X-Last-TLS-Session-Version: TLSv1.3 Hello Tomi, On Thu Jun 19, 2025 at 2:27 PM CEST, Tomi Valkeinen wrote: > The commit c9b1150a68d9 ("drm/atomic-helper: Re-order bridge chain > pre-enable and post-disable") changed the order of enable/disable calls. > Previously the calls (on imx8mm) were: > > mxsfb_crtc_atomic_enable() > samsung_dsim_atomic_pre_enable() > samsung_dsim_atomic_enable() > > now the order is: > > samsung_dsim_atomic_pre_enable() > mxsfb_crtc_atomic_enable() > samsung_dsim_atomic_enable() > > On imx8mm (possibly on imx8mp, and other platforms too) this causes two > issues: > > 1. The DSI PLL setup depends on a refclk, but the DSI driver does not > set the rate, just uses it with the rate it has. On imx8mm this refclk > seems to be related to the LCD controller's video clock. So, when the > mxsfb driver sets its video clock, DSI's refclk rate changes. > > Earlier this mxsfb_crtc_atomic_enable() set the video clock, so the PLL > refclk rate was set (and didn't change) in the DSI enable calls. Now the > rate changes between DSI's pre_enable() and enable(), but the driver > configures the PLL in the pre_enable(). > > Thus you get a black screen on a modeset. Doing the modeset again works, > as the video clock rate stays the same. > > 2. The image on the screen is shifted/wrapped horizontally. I have not > found the exact reason for this, but the documentation seems to hint > that the LCD controller's pixel stream should be enabled first, before > setting up the DSI. This would match the change, as now the pixel stream > starts only after DSI driver's pre_enable(). > > The main function related to this issue is samsung_dsim_init() which > will do the clock and link configuration. samsung_dsim_init() is > currently called from pre_enable(), but it is also called from > samsung_dsim_host_transfer() to set up the link if the peripheral driver > wants to send a DSI command. > > This patch fixes both issues by moving the samsung_dsim_init() call from > pre_enable() to enable(). > > However, to deal with the case where the samsung_dsim_init() has already > been called from samsung_dsim_host_transfer() and the refclk rate has > changed, we need to make sure we re-initialize the DSI with the new rate > in enable(). This is achieved by clearing the DSIM_STATE_INITIALIZED > flag and uninitializing the clocks and irqs before calling > samsung_dsim_init(). > > Fixes: c9b1150a68d9 ("drm/atomic-helper: Re-order bridge chain pre-enable= and post-disable") > Reported-by: Hiago De Franco > Signed-off-by: Tomi Valkeinen Is this old patch still valid, or outdated/superseded? Assuming it is still valid, I tested on an i.MX8MP and I'm afraid my display stops working. :-( My pipeline is: i.MX8MP LCDIF -> samsung-dsim -> TI SN65DSI84 -> dual-LVDS panel Using either 'modetest -s' or weston, the result is: * backlight turns on * the TI bridge logs: sn65dsi83 4-002c: failed to lock PLL, ret=3D-110 sn65dsi83 4-002c: Unexpected link status 0x01 * panel stays black Running multiple tests one after another (e.g. modetest -s, exit, modetest -s, exit, repeat) the display keeps on staying black. In other words the "Doing the modeset again works, as the video clock rate stays the same" does not seem to apply here. I haven't investigated further but I'm available to do any specific test you may suggest. I have an additional question about your patch, see below. > --- a/drivers/gpu/drm/bridge/samsung-dsim.c > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c > @@ -1473,22 +1473,31 @@ static void samsung_dsim_atomic_pre_enable(struct= drm_bridge *bridge, > } > > dsi->state |=3D DSIM_STATE_ENABLED; > - > - /* > - * For Exynos-DSIM the downstream bridge, or panel are expecting > - * the host initialization during DSI transfer. > - */ > - if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) { > - ret =3D samsung_dsim_init(dsi); > - if (ret) > - return; > - } The code being removed here is only for the non-exynos case... > } > > static void samsung_dsim_atomic_enable(struct drm_bridge *bridge, > struct drm_atomic_state *state) > { > struct samsung_dsim *dsi =3D bridge_to_dsi(bridge); > + int ret; > + > + /* > + * The DSI bridge may have already been initialized in > + * samsung_dsim_host_transfer(). It is possible that the refclk rate ha= s > + * changed after that due to the display controller configuration, and > + * thus we need to reinitialize the DSI bridge to ensure the correct > + * clock settings. > + */ > + > + if (dsi->state & DSIM_STATE_INITIALIZED) { > + dsi->state &=3D ~DSIM_STATE_INITIALIZED; > + samsung_dsim_disable_clock(dsi); > + samsung_dsim_disable_irq(dsi); > + } > + > + ret =3D samsung_dsim_init(dsi); > + if (ret) > + return; ...but the added code is for all variants. Is this correct? Note this is not the cause of the problem I reported with the i.MX8MP because thats a non-exynos case anyway. Luca -- Luca Ceresoli, Bootlin Embedded Linux and Kernel engineering https://bootlin.com