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

["Luca Ceresoli" <luca.ceresoli@bootlin.com>]

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 <hiagofranco@gmail.com>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

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=-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 |= 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 = 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 = 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 has
> +	 * 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 &= ~DSIM_STATE_INITIALIZED;
> +		samsung_dsim_disable_clock(dsi);
> +		samsung_dsim_disable_irq(dsi);
> +	}
> +
> +	ret = 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