* Re: [PATCH] drm/bridge: samsung-dsim: Fix init order
2025-06-19 12:27 ` [PATCH] drm/bridge: samsung-dsim: Fix init order Tomi Valkeinen
@ 2025-06-20 12:33 ` Hiago De Franco
2025-06-23 9:34 ` Marek Szyprowski
` (3 subsequent siblings)
4 siblings, 0 replies; 8+ messages in thread
From: Hiago De Franco @ 2025-06-20 12:33 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: 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, dri-devel, linux-kernel, Francesco Dolcini,
Hiago De Franco
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>
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] drm/bridge: samsung-dsim: Fix init order
2025-06-19 12:27 ` [PATCH] drm/bridge: samsung-dsim: Fix init order Tomi Valkeinen
2025-06-20 12:33 ` Hiago De Franco
@ 2025-06-23 9:34 ` Marek Szyprowski
2025-06-23 10:02 ` Francesco Dolcini
2025-09-25 13:53 ` Jan Remmet
` (2 subsequent siblings)
4 siblings, 1 reply; 8+ messages in thread
From: Marek Szyprowski @ 2025-06-23 9:34 UTC (permalink / raw)
To: Tomi Valkeinen, Inki Dae, Jagan Teki, 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
Cc: dri-devel, linux-kernel, Hiago De Franco, Francesco Dolcini
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.
> ---
> 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,
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] drm/bridge: samsung-dsim: Fix init order
2025-06-23 9:34 ` Marek Szyprowski
@ 2025-06-23 10:02 ` Francesco Dolcini
0 siblings, 0 replies; 8+ messages in thread
From: Francesco Dolcini @ 2025-06-23 10:02 UTC (permalink / raw)
To: Marek Szyprowski
Cc: Tomi Valkeinen, Inki Dae, Jagan Teki, 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, dri-devel, linux-kernel, Hiago De Franco,
Francesco Dolcini
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/bridge: samsung-dsim: Fix init order
2025-06-19 12:27 ` [PATCH] drm/bridge: samsung-dsim: Fix init order Tomi Valkeinen
2025-06-20 12:33 ` Hiago De Franco
2025-06-23 9:34 ` Marek Szyprowski
@ 2025-09-25 13:53 ` Jan Remmet
2025-10-22 7:24 ` Tomi Valkeinen
2026-02-03 12:42 ` Luca Ceresoli
4 siblings, 0 replies; 8+ messages in thread
From: Jan Remmet @ 2025-09-25 13:53 UTC (permalink / raw)
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
Cc: dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
Hiago De Franco, Francesco Dolcini
Am 19.06.25 um 14:27 schrieb Tomi Valkeinen:
> 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>
I run also in this issue and tried as similar solution.
Tested on imx8mm-phyboard-polis with a sn65dsi83 MIPI to LVDS converter [1]
Tested-by: Jan Remmet <j.remmet@phytec.de>
This issue still persists on current master.
Best regards Jan
[1]
https://lore.kernel.org/all/20250925-wip-j-remmet-phytec-de-bspimx8m-3801_peb-av-10_with_ac209-v1-0-94f9f775ee62@phytec.de/
> ---
> 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,
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] drm/bridge: samsung-dsim: Fix init order
2025-06-19 12:27 ` [PATCH] drm/bridge: samsung-dsim: Fix init order Tomi Valkeinen
` (2 preceding siblings ...)
2025-09-25 13:53 ` Jan Remmet
@ 2025-10-22 7:24 ` Tomi Valkeinen
2026-02-03 12:42 ` Luca Ceresoli
4 siblings, 0 replies; 8+ messages in thread
From: Tomi Valkeinen @ 2025-10-22 7:24 UTC (permalink / raw)
To: 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
Cc: dri-devel, linux-kernel, Hiago De Franco, Francesco Dolcini
Hi samsung-dsim maintainers,
On 19/06/2025 15: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>
Can this be merged?
Tomi
> ---
> 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,
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] drm/bridge: samsung-dsim: Fix init order
2025-06-19 12:27 ` [PATCH] drm/bridge: samsung-dsim: Fix init order Tomi Valkeinen
` (3 preceding siblings ...)
2025-10-22 7:24 ` Tomi Valkeinen
@ 2026-02-03 12:42 ` Luca Ceresoli
2026-02-03 12:51 ` Tomi Valkeinen
4 siblings, 1 reply; 8+ messages in thread
From: Luca Ceresoli @ 2026-02-03 12:42 UTC (permalink / raw)
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
Cc: dri-devel, linux-kernel, Hiago De Franco, Francesco Dolcini
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] drm/bridge: samsung-dsim: Fix init order
2026-02-03 12:42 ` Luca Ceresoli
@ 2026-02-03 12:51 ` Tomi Valkeinen
0 siblings, 0 replies; 8+ messages in thread
From: Tomi Valkeinen @ 2026-02-03 12:51 UTC (permalink / raw)
To: Luca Ceresoli
Cc: dri-devel, linux-kernel, Hiago De Franco, Francesco Dolcini,
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
Hi,
On 03/02/2026 14:42, Luca Ceresoli wrote:
> 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?
No, not valid, ignore it =).
The enable/disable sequence was restored to as it was before.
Tomi
^ permalink raw reply [flat|nested] 8+ messages in thread