[PATCH RFC 0/2] drm/bridge: re order of bridge chain causes flickering display

[PATCH RFC 0/2] drm/bridge: re order of bridge chain causes flickering display

[Jan Remmet]

commit c9b1150a68d9 ("drm/atomic-helper: Re-order bridge chain pre-enable
and post-disable") caused our display [1] to flicker constantly.

Our setup is
i.MX8MM mxsfb -> samsung_dsim ->
 sn65dsi83 (MIPI to LVDS) -> ETML1010G3DRA (display)

Reverting the commit [2] let the display work again.
Only difference in clk_summary is the change of
dsi_phy_ref from 72400000 (flickering) to 18100000 (working)

The same behavior can be seen with a ph128800t006-zhc01 display
and dsi_phy_ref from 66500000 (flickering) to 16625000 (working)

The root cause seems to be that mxsfb_crtc_atomic_enable is called after
samsung_dsim_atomic_pre_enable. This changes the parent clock after
the PLL is already configured.

According to the commit [2] pre_enable hooks should not rely on clocks.
Both drivers used in our setup work with clocks in pre_enable.

Moving the clock setup from pre_enable to enable fix the issue for
our setup.

Please note that I am not deeply familiar with the DRM subsystem internals,
and there might be side effects of this change that I haven't considered.

[1] arch/arm64/boot/dts/freescale/imx8mm-phyboard-polis-peb-av-10.dtso
[2] https://lore.kernel.org/all/20250605171524.27222-4-aradhya.bhatia@linux.dev/

Signed-off-by: Jan Remmet <j.remmet@phytec.de>
---
Jan Remmet (2):
      drm/bridge: ti-sn65dsi83: move clk setup to enable
      drm/bridge: samsung-dsim: move clk setup to enable

 drivers/gpu/drm/bridge/samsung-dsim.c | 14 ++++++++------
 drivers/gpu/drm/bridge/ti-sn65dsi83.c | 16 ++++++++--------
 2 files changed, 16 insertions(+), 14 deletions(-)
---
base-commit: bf40f4b87761e2ec16efc8e49b9ca0d81f4115d8
change-id: 20250925-wip-j-remmet-phytec-de-bspimx8m-3801_peb-av-10_with_ac209-058abe8477c6

Best regards,
-- 
Jan Remmet <j.remmet@phytec.de>

[PATCH RFC 1/2] drm/bridge: ti-sn65dsi83: move clk setup to enable

[Jan Remmet]

Move enable PLL from pre_enable to sn65dsi83_atomic_pre_enable.

The base clocks may not be ready when pre_enable is called.

commit c9b1150a68d9 ("drm/atomic-helper: Re-order bridge chain pre-enable
		and post-disable") points out that pre_enable hook definition says that
"The display pipe (i.e. clocks and timing signals) feeding this bridge
will not yet be running when this callback is called".

Signed-off-by: Jan Remmet <j.remmet@phytec.de>
---
 drivers/gpu/drm/bridge/ti-sn65dsi83.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
index 033c44326552ab167d4e8d9b74957c585e4c6fb7..4cbdbafc3e85821ef6fcd9f74bf07faf6b9f84ff 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
@@ -474,7 +474,6 @@ static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge,
 	struct drm_crtc *crtc;
 	bool lvds_format_24bpp;
 	bool lvds_format_jeida;
-	unsigned int pval;
 	__le16 le16val;
 	u16 val;
 	int ret;
@@ -614,6 +613,14 @@ static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge,
 	regmap_write(ctx->regmap, REG_VID_CHA_VERTICAL_FRONT_PORCH,
 		     mode->vsync_start - mode->vdisplay);
 	regmap_write(ctx->regmap, REG_VID_CHA_TEST_PATTERN, 0x00);
+}
+
+static void sn65dsi83_atomic_enable(struct drm_bridge *bridge,
+				    struct drm_atomic_state *state)
+{
+	struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
+	unsigned int pval;
+	int ret;
 
 	/* Enable PLL */
 	regmap_write(ctx->regmap, REG_RC_PLL_EN, REG_RC_PLL_EN_PLL_EN);
@@ -633,13 +640,6 @@ static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge,
 
 	/* Wait for 10ms after soft reset as specified in datasheet */
 	usleep_range(10000, 12000);
-}
-
-static void sn65dsi83_atomic_enable(struct drm_bridge *bridge,
-				    struct drm_atomic_state *state)
-{
-	struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
-	unsigned int pval;
 
 	/* Clear all errors that got asserted during initialization. */
 	regmap_read(ctx->regmap, REG_IRQ_STAT, &pval);

-- 
2.43.0

[PATCH RFC 2/2] drm/bridge: samsung-dsim: move clk setup to enable

[Jan Remmet]

Move the samsung_dsim_init and so samsung_dsim_enable_clock to
samsung_dsim_atomic_enable.

The base clocks may not be ready when pre_enable is called.

commit c9b1150a68d9 ("drm/atomic-helper: Re-order bridge chain pre-enable
and post-disable") points out that pre_enable hook definition says that
"The display pipe (i.e. clocks and timing signals) feeding this bridge
will not yet be running when this callback is called".

Signed-off-by: Jan Remmet <j.remmet@phytec.de>
---
 drivers/gpu/drm/bridge/samsung-dsim.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
index b5dd71f6a99057d98cf15090f6081d0460836ec5..53ce3228586ac0f6abfa252b7c15d9eba459c96c 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -1460,6 +1460,14 @@ static void samsung_dsim_atomic_pre_enable(struct drm_bridge *bridge,
 	}
 
 	dsi->state |= DSIM_STATE_ENABLED;
+}
+
+static void samsung_dsim_atomic_enable(struct drm_bridge *bridge,
+				       struct drm_atomic_state *state)
+{
+	pr_debug("%s called from %pS\n", __func__, __builtin_return_address(0));
+	struct samsung_dsim *dsi = bridge_to_dsi(bridge);
+	int ret;
 
 	/*
 	 * For Exynos-DSIM the downstream bridge, or panel are expecting
@@ -1470,12 +1478,6 @@ static void samsung_dsim_atomic_pre_enable(struct drm_bridge *bridge,
 		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);
 
 	samsung_dsim_set_display_mode(dsi);
 	samsung_dsim_set_display_enable(dsi, true);

-- 
2.43.0

Re: [PATCH RFC 2/2] drm/bridge: samsung-dsim: move clk setup to enable

[Marek Szyprowski]

On 25.09.2025 13:35, Jan Remmet wrote:
> Move the samsung_dsim_init and so samsung_dsim_enable_clock to
> samsung_dsim_atomic_enable.
>
> The base clocks may not be ready when pre_enable is called.
>
> commit c9b1150a68d9 ("drm/atomic-helper: Re-order bridge chain pre-enable
> and post-disable") points out that pre_enable hook definition says that
> "The display pipe (i.e. clocks and timing signals) feeding this bridge
> will not yet be running when this callback is called".
>
> Signed-off-by: Jan Remmet <j.remmet@phytec.de>

Isn't this similar to this 
patch https://lore.kernel.org/all/20250619-samsung-dsim-fix-v1-1-6b5de68fb115@ideasonboard.com/ 
?


> ---
>   drivers/gpu/drm/bridge/samsung-dsim.c | 14 ++++++++------
>   1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index b5dd71f6a99057d98cf15090f6081d0460836ec5..53ce3228586ac0f6abfa252b7c15d9eba459c96c 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -1460,6 +1460,14 @@ static void samsung_dsim_atomic_pre_enable(struct drm_bridge *bridge,
>   	}
>   
>   	dsi->state |= DSIM_STATE_ENABLED;
> +}
> +
> +static void samsung_dsim_atomic_enable(struct drm_bridge *bridge,
> +				       struct drm_atomic_state *state)
> +{
> +	pr_debug("%s called from %pS\n", __func__, __builtin_return_address(0));
> +	struct samsung_dsim *dsi = bridge_to_dsi(bridge);
> +	int ret;
>   
>   	/*
>   	 * For Exynos-DSIM the downstream bridge, or panel are expecting
> @@ -1470,12 +1478,6 @@ static void samsung_dsim_atomic_pre_enable(struct drm_bridge *bridge,
>   		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);
>   
>   	samsung_dsim_set_display_mode(dsi);
>   	samsung_dsim_set_display_enable(dsi, true);
>
Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH RFC 2/2] drm/bridge: samsung-dsim: move clk setup to enable

[Jan Remmet]

Am 25.09.25 um 13:47 schrieb Marek Szyprowski:
> On 25.09.2025 13:35, Jan Remmet wrote:
>> Move the samsung_dsim_init and so samsung_dsim_enable_clock to
>> samsung_dsim_atomic_enable.
>>
>> The base clocks may not be ready when pre_enable is called.
>>
>> commit c9b1150a68d9 ("drm/atomic-helper: Re-order bridge chain pre-enable
>> and post-disable") points out that pre_enable hook definition says that
>> "The display pipe (i.e. clocks and timing signals) feeding this bridge
>> will not yet be running when this callback is called".
>>
>> Signed-off-by: Jan Remmet <j.remmet@phytec.de>
> 
> Isn't this similar to this
> patch https://lore.kernel.org/all/20250619-samsung-dsim-fix-v1-1-6b5de68fb115@ideasonboard.com/
> ?

Indeed thanks for pointing to it. I'll try this together with my 
ti-sn65dsi83 changes and send a Tested-by.
I wonder why it is still on the list.

Jan
> 
> 
>> ---
>>    drivers/gpu/drm/bridge/samsung-dsim.c | 14 ++++++++------
>>    1 file changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
>> index b5dd71f6a99057d98cf15090f6081d0460836ec5..53ce3228586ac0f6abfa252b7c15d9eba459c96c 100644
>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
>> @@ -1460,6 +1460,14 @@ static void samsung_dsim_atomic_pre_enable(struct drm_bridge *bridge,
>>    	}
>>    
>>    	dsi->state |= DSIM_STATE_ENABLED;
>> +}
>> +
>> +static void samsung_dsim_atomic_enable(struct drm_bridge *bridge,
>> +				       struct drm_atomic_state *state)
>> +{
>> +	pr_debug("%s called from %pS\n", __func__, __builtin_return_address(0));
>> +	struct samsung_dsim *dsi = bridge_to_dsi(bridge);
>> +	int ret;
>>    
>>    	/*
>>    	 * For Exynos-DSIM the downstream bridge, or panel are expecting
>> @@ -1470,12 +1478,6 @@ static void samsung_dsim_atomic_pre_enable(struct drm_bridge *bridge,
>>    		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);
>>    
>>    	samsung_dsim_set_display_mode(dsi);
>>    	samsung_dsim_set_display_enable(dsi, true);
>>
> Best regards