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

Hiago De Franco
2025-06-20T12:34:00Z

On Thu, Jun 19, 2025 at 03:27:18PM +0300, 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>

Tested-by: Hiago De Franco <hiago.franco@toradex.com> # Verdin iMX8MP

Best regards,
Hiago.

> ---
> drivers/gpu/drm/bridge/samsung-dsim.c | 29 +++++++++++++++++++----------
> 1 file changed, 19 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index f2f666b27d2d..cec383d8946d 100644
> --- 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;
> - }
> }
>
> 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;
>
> samsung_dsim_set_display_mode(dsi);
> samsung_dsim_set_display_enable(dsi, true);
>
> ---
> base-commit: 7872997c048e989c7689c2995d230fdca7798000
> change-id: 20250619-samsung-dsim-fix-58c8ec8193e9
>
> Best regards,
> --
> Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>