Re: [PATCH] drm/bridge: samsung-dsim: Fix init order

[Francesco Dolcini <francesco@dolcini.it>]

On Mon, Jun 23, 2025 at 11:34:34AM +0200, Marek Szyprowski wrote:
> On 19.06.2025 14:27, 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 <hiagofranco@gmail.com>
> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> 
> Seems to be working fine on all my Exynos based boards:
> 
> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> 
> 
> BTW, it was a long discussion how to handle the dsim initialization and 
> we agreed to keep calling samsung_dsim_init() on first dsi transfer for 
> Exynos case and from pre-enable for others:
> 
> https://lore.kernel.org/all/20221209152343.180139-11-jagan@amarulasolutions.com/
> 
> I'm not sure if changing this won't break again something, especially 
> the boards with DSI bridge or panel controlled via I2C instead of the 
> DSI commands. This has to be tested on the all supported variants of 
> this hardware.

FWIW, DSI bridges (LT8912B and SN65DSI83) controlled over I2C were
tested fine with this patch on both NXP i.MX8MP and 8MM (see Hiago
tested-by).


Francesco