[PATCH v3 0/3] Fixes and enhancements for Exynos (7870) DSIM bridge driver

[PATCH v3 0/3] Fixes and enhancements for Exynos (7870) DSIM bridge driver

[Kaustabh Chakraborty]

Since v6.17, there is a regression for the Exynos 7870 DSIM driver. The
display occasionally had random aberration when the panel was turned on.
The first patch addresses that.

The second patch replaces an implicit loop for waiting for PLL
stabilization with an interrupt-based solution, which should be more
reliable. This solution was suggested by Inki Dae in a discussion of an
earlier patch series sent by me. For further details, refer to its
commit description.

Signed-off-by: Kaustabh Chakraborty <kauschluss@disroot.org>
---
Changes in v3:
- Add patch for removing conflicting devices for DRM (Sam Day)
- Link to v2: https://lore.kernel.org/r/20260208-exynos-dsim-fixes-v2-0-a857e8130a2a@disroot.org

Changes in v2:
- drop now-not-required [v1 1/3] (Marek Szyprowski)
- Link to v1: https://lore.kernel.org/r/20260124-exynos-dsim-fixes-v1-0-122d047a23d1@disroot.org

---
Kaustabh Chakraborty (3):
      drm/bridge: samsung-dsim: enable MFLUSH_VS for Exynos 7870 DSIM
      drm/bridge: samsung-dsim: use DSIM interrupt to wait for PLL stability
      drm/exynos: remove simplefb nodes before init

 drivers/gpu/drm/bridge/samsung-dsim.c   | 48 +++++++++++++++++++++++----------
 drivers/gpu/drm/exynos/exynos_drm_drv.c |  6 +++++
 include/drm/bridge/samsung-dsim.h       |  1 +
 3 files changed, 41 insertions(+), 14 deletions(-)
---
base-commit: 290aaf24a551d5a0dce037e3fab30820f9113a10
change-id: 20260124-exynos-dsim-fixes-5383d6a6f073

Best regards,
--  
Kaustabh Chakraborty <kauschluss@disroot.org>

[PATCH v3 1/3] drm/bridge: samsung-dsim: enable MFLUSH_VS for Exynos 7870 DSIM

[Kaustabh Chakraborty]

Commit a36c533ad3e1 ("drm/bridge: samsung-dsim: Always flush display
FIFO on vsync pulse") intends to enable FIFO flushing at v-sync pulse by
not setting the active-low MFLUSH_VS bit.

However, in Exynos 7870 DSIM, the MFLUSH_VS bit is active-high. There is
no publicly available documentation to the best of my knowledge, but
downstream kernel code [1] supports this claim. Enable the bit for
Exynos 7870.

Cc: stable@vger.kernel.org # v6.17 and later
Link: https://github.com/samsungexynos7870/android_kernel_samsung_exynos7870/blob/a3762bb1761ae/drivers/video/fbdev/exynos/decon_7870/dsim_reg_7870.c#L699 [1]
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Kaustabh Chakraborty <kauschluss@disroot.org>
---
 drivers/gpu/drm/bridge/samsung-dsim.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
index 25ab475309e8..da753ff6eed4 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -1089,6 +1089,13 @@ static int samsung_dsim_init_link(struct samsung_dsim *dsi)
 			reg |= DSIM_HBP_DISABLE_MODE;
 		if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_NO_HSA)
 			reg |= DSIM_HSA_DISABLE_MODE;
+
+		/*
+		 * For some hardware types, DSIM_MFLUSH_VS bit needs to be
+		 * enabled explicitly.
+		 */
+		if (dsi->plat_data->hw_type == DSIM_TYPE_EXYNOS7870)
+			reg |= DSIM_MFLUSH_VS;
 	}
 
 	if (dsi->mode_flags & MIPI_DSI_MODE_NO_EOT_PACKET)

-- 
2.54.0

Re: [PATCH v3 1/3] drm/bridge: samsung-dsim: enable MFLUSH_VS for Exynos 7870 DSIM

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The macro `samsung_dsim_hw_is_exynos()` incorrectly evaluates to false for `DSIM_TYPE_EXYNOS7870`, causing Exynos 7870 to be treated as non-Exynos hardware.
--

--- Patch [1]: [PATCH v3 1/3] drm/bridge: samsung-dsim: enable MFLUSH_VS for Exynos 7870 DSIM ---
commit fb46175bd3153fc02fa44c4333e69d1b1ac4e3b4
Author: Kaustabh Chakraborty <kauschluss@disroot.org>
Subject: drm/bridge: samsung-dsim: enable MFLUSH_VS for Exynos 7870 DSIM

This commit enables FIFO flushing at v-sync pulse by setting the MFLUSH_VS
bit for Exynos 7870. The bit is active-high for this hardware variant.

Link: https://github.com/samsungexynos7870/android_kernel_samsung_exynos7870/blob/a3762bb1761ae/drivers/video/fbdev/exynos/decon_7870/dsim_reg_7870.c#L699 [1]

> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index 25ab475309e87..da753ff6eed4d 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -1089,6 +1089,13 @@ static int samsung_dsim_init_link(struct samsung_dsim *dsi)
>  			reg |= DSIM_HBP_DISABLE_MODE;
>  		if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_NO_HSA)
>  			reg |= DSIM_HSA_DISABLE_MODE;
> +
> +		/*
> +		 * For some hardware types, DSIM_MFLUSH_VS bit needs to be
> +		 * enabled explicitly.
> +		 */
> +		if (dsi->plat_data->hw_type == DSIM_TYPE_EXYNOS7870)
> +			reg |= DSIM_MFLUSH_VS;

[Severity: High]
This is a pre-existing issue, but does the DSIM_TYPE_EXYNOS7870 hardware type
need to be included in the samsung_dsim_hw_is_exynos() macro?

Currently, the macro in include/drm/bridge/samsung-dsim.h is defined as:

#define samsung_dsim_hw_is_exynos(hw) \
	((hw) >= DSIM_TYPE_EXYNOS3250 && (hw) <= DSIM_TYPE_EXYNOS5433)

Because Exynos 7870 falls outside this range, the macro evaluates to false
for it, causing the driver to treat it as non-Exynos hardware.

This causes incorrect configurations later in samsung_dsim_init_link():

	if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
		reg |= DSIM_NON_CONTINUOUS_CLKLANE;

And in samsung_dsim_atomic_pre_enable(), it causes the wrong initialization
path to be taken:

	if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
		ret = samsung_dsim_init(dsi);
		if (ret)
			return;
	}

Could this cause initialization ordering issues or incorrect hardware
configuration for Exynos 7870?

>  	}
>  
>  	if (dsi->mode_flags & MIPI_DSI_MODE_NO_EOT_PACKET)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723-exynos-dsim-fixes-v3-0-0c31ae1dbecc@disroot.org?part=1

[PATCH v3 2/3] drm/bridge: samsung-dsim: use DSIM interrupt to wait for PLL stability

[Kaustabh Chakraborty]

Stabilizing PLL needs to be waited for. This is done using a loop,
checking the PLL_STABLE bit in the status register.

DSIM fires an interrupt when the PLL is stabilized. Rely on this
functionality for stabilization wait, getting rid of the implicit loop.

This has been tested on a Galaxy J6 (Exynos 7870). Unfortunately, since
testing on all supported devices is less feasible, introduce a stop-gap
measure where the timeout has a gracious lower bound of 100
microseconds. This will (hopefully) prevent regressions due to timeout
on other devices.

Suggested-by: Inki Dae <inki.dae@samsung.com>
Link: https://lore.kernel.org/r/CAAQKjZMLMbwDVZRb5+Xb_5yz3AEP4uuzFJMuuZy9NFDu13VU5w@mail.gmail.com
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Kaustabh Chakraborty <kauschluss@disroot.org>
---
 drivers/gpu/drm/bridge/samsung-dsim.c | 41 +++++++++++++++++++++++------------
 include/drm/bridge/samsung-dsim.h     |  1 +
 2 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
index da753ff6eed4..866cff205e71 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -17,6 +17,7 @@
 #include <linux/export.h>
 #include <linux/irq.h>
 #include <linux/media-bus-format.h>
+#include <linux/minmax.h>
 #include <linux/of.h>
 #include <linux/phy/phy.h>
 #include <linux/platform_device.h>
@@ -788,7 +789,7 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi,
 {
 	const struct samsung_dsim_driver_data *driver_data = dsi->driver_data;
 	unsigned long fin, fout;
-	int timeout;
+	unsigned int timeout;
 	u8 p, s;
 	u16 m;
 	u32 reg;
@@ -849,19 +850,26 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi,
 	if (dsi->swap_dn_dp_data)
 		reg |= DSIM_PLL_DPDNSWAP_DAT;
 
+	/*
+	 * The PLL_TIMER value is the product of the timeout delay and the APB
+	 * bus clock rate. Calcutate the timeout delay on-the-fly here.
+	 * It is assumed that the bus clock is the first clock in the provided
+	 * bulk clock data.
+	 */
+	timeout = 100;
+	fin = clk_get_rate(dsi->driver_data->clk_data[0].clk) / HZ_PER_MHZ;
+	if (fin)
+		timeout = max(dsi->driver_data->reg_values[PLL_TIMER] / fin,
+			      timeout);
+
+	reinit_completion(&dsi->pll_stabilized);
 	samsung_dsim_write(dsi, DSIM_PLLCTRL_REG, reg);
 
-	timeout = 3000;
-	do {
-		if (timeout-- == 0) {
-			dev_err(dsi->dev, "PLL failed to stabilize\n");
-			return 0;
-		}
-		if (driver_data->has_legacy_status_reg)
-			reg = samsung_dsim_read(dsi, DSIM_STATUS_REG);
-		else
-			reg = samsung_dsim_read(dsi, DSIM_LINK_STATUS_REG);
-	} while ((reg & BIT(driver_data->pll_stable_bit)) == 0);
+	if (wait_for_completion_timeout(&dsi->pll_stabilized,
+					usecs_to_jiffies(timeout))) {
+		dev_err(dsi->dev, "PLL failed to stabilize\n");
+		return 0;
+	}
 
 	dsi->hs_clock = fout;
 
@@ -1596,8 +1604,12 @@ static irqreturn_t samsung_dsim_irq(int irq, void *dev_id)
 		return IRQ_HANDLED;
 	}
 
-	if (!(status & (DSIM_INT_RX_DONE | DSIM_INT_SFR_FIFO_EMPTY |
-			DSIM_INT_PLL_STABLE)))
+	if (status & DSIM_INT_PLL_STABLE) {
+		complete(&dsi->pll_stabilized);
+		return IRQ_HANDLED;
+	}
+
+	if (!(status & (DSIM_INT_RX_DONE | DSIM_INT_SFR_FIFO_EMPTY)))
 		return IRQ_HANDLED;
 
 	if (samsung_dsim_transfer_finish(dsi))
@@ -2148,6 +2160,7 @@ int samsung_dsim_probe(struct platform_device *pdev)
 		return PTR_ERR(dsi);
 
 	init_completion(&dsi->completed);
+	init_completion(&dsi->pll_stabilized);
 	spin_lock_init(&dsi->transfer_lock);
 	INIT_LIST_HEAD(&dsi->transfer_list);
 
diff --git a/include/drm/bridge/samsung-dsim.h b/include/drm/bridge/samsung-dsim.h
index 03005e474704..e3433da21ad0 100644
--- a/include/drm/bridge/samsung-dsim.h
+++ b/include/drm/bridge/samsung-dsim.h
@@ -123,6 +123,7 @@ struct samsung_dsim {
 	int state;
 	struct drm_property *brightness;
 	struct completion completed;
+	struct completion pll_stabilized;
 
 	spinlock_t transfer_lock; /* protects transfer_list */
 	struct list_head transfer_list;

-- 
2.54.0

Re: [PATCH v3 2/3] drm/bridge: samsung-dsim: use DSIM interrupt to wait for PLL stability

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] The return value of wait_for_completion_timeout() is evaluated incorrectly, treating a timeout as success and success as an error.
- [High] The DSIM_INT_PLL_STABLE interrupt is permanently masked in DSIM_INTMSK_REG and will never fire.
- [Medium] Using `wait_for_completion_timeout` with a 1-jiffy timeout can expire prematurely, violating hardware timing requirements.

Pre-existing issues:
- [High] Global static clk_bulk_data structures are shared among driver instances, leading to pointer overwrites and potential Use-After-Free.
--

--- Patch [2]: [PATCH v3 2/3] drm/bridge: samsung-dsim: use DSIM interrupt to wait for PLL stability ---
commit ab2f26b528640a4bd75a69b30c4182a9e493647b
Author: Kaustabh Chakraborty <kauschluss@disroot.org>

drm/bridge: samsung-dsim: use DSIM interrupt to wait for PLL stability

Stabilizing PLL needs to be waited for. This is done using a loop,
checking the PLL_STABLE bit in the status register. DSIM fires an
interrupt when the PLL is stabilized. Rely on this functionality for
stabilization wait, getting rid of the implicit loop.

Link: https://lore.kernel.org/r/CAAQKjZMLMbwDVZRb5+Xb_5yz3AEP4uuzFJMuuZy9NFDu13VU5w@mail.gmail.com

> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c

[ ... ]

> @@ -849,19 +850,26 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi,
>  	if (dsi->swap_dn_dp_data)
>  		reg |= DSIM_PLL_DPDNSWAP_DAT;
>  
> +	/*
> +	 * The PLL_TIMER value is the product of the timeout delay and the APB
> +	 * bus clock rate. Calcutate the timeout delay on-the-fly here.
> +	 * It is assumed that the bus clock is the first clock in the provided
> +	 * bulk clock data.
> +	 */
> +	timeout = 100;
> +	fin = clk_get_rate(dsi->driver_data->clk_data[0].clk) / HZ_PER_MHZ;

[Severity: High]
This is a pre-existing issue, but does accessing the global static
clk_bulk_data structures here risk a use-after-free or pointer overwrite
in multi-DSI systems?

The driver defines global static arrays that dsi->driver_data->clk_data points
to, and devm_clk_bulk_get() modifies this shared array in-place, overwriting
the .clk pointers. In multi-DSI systems, a second instance's probe will
overwrite the clock pointers belonging to the first. If an instance is unbound,
the pointers become dangling.

Could this dereference of the potentially overwritten or dangling clock pointer
cause issues during PLL setup?

> +	if (fin)
> +		timeout = max(dsi->driver_data->reg_values[PLL_TIMER] / fin,
> +			      timeout);
> +
> +	reinit_completion(&dsi->pll_stabilized);
>  	samsung_dsim_write(dsi, DSIM_PLLCTRL_REG, reg);
>  
> -	timeout = 3000;
> -	do {
> -		if (timeout-- == 0) {
> -			dev_err(dsi->dev, "PLL failed to stabilize\n");
> -			return 0;
> -		}
> -		if (driver_data->has_legacy_status_reg)
> -			reg = samsung_dsim_read(dsi, DSIM_STATUS_REG);
> -		else
> -			reg = samsung_dsim_read(dsi, DSIM_LINK_STATUS_REG);
> -	} while ((reg & BIT(driver_data->pll_stable_bit)) == 0);
> +	if (wait_for_completion_timeout(&dsi->pll_stabilized,
> +					usecs_to_jiffies(timeout))) {

[Severity: Medium]
Since the timeout is dynamically calculated with a lower bound of 100
microseconds, usecs_to_jiffies(100) will evaluate to 1 jiffy on most
systems.

Does a 1-jiffy timeout mean 'timeout on the next timer tick', which could
happen almost immediately depending on timer alignment?

Could this cause the wait to expire prematurely before the hardware PLL
has actually stabilized, leading to regressions?

[Severity: High]
Does this code incorrectly handle the return value of
wait_for_completion_timeout()?

wait_for_completion_timeout() returns 0 if the wait timed out, and > 0 if
it was successful. Evaluating the return value directly in the if condition
treats > 0 as true, causing a successful wait to erroneously execute the
error path and return 0 (failure).

Conversely, if it times out, it returns 0, evaluating to false, and
incorrectly returns success.

> +		dev_err(dsi->dev, "PLL failed to stabilize\n");
> +		return 0;
> +	}
>  
>  	dsi->hs_clock = fout;

[ ... ]

> @@ -1596,8 +1604,12 @@ static irqreturn_t samsung_dsim_irq(int irq, void *dev_id)
>  		return IRQ_HANDLED;
>  	}
>  
> -	if (!(status & (DSIM_INT_RX_DONE | DSIM_INT_SFR_FIFO_EMPTY |
> -			DSIM_INT_PLL_STABLE)))
> +	if (status & DSIM_INT_PLL_STABLE) {
> +		complete(&dsi->pll_stabilized);
> +		return IRQ_HANDLED;
> +	}

[Severity: High]
Will the DSIM_INT_PLL_STABLE interrupt ever fire?

Looking earlier in samsung_dsim_irq(), when DSIM_INT_SW_RST_RELEASE is
handled, the hardware interrupt mask is rewritten using:

    unsigned long mask = ~(DSIM_INT_RX_DONE |
                           DSIM_INT_SFR_FIFO_EMPTY |
                           DSIM_INT_SFR_HDR_FIFO_EMPTY |
                           DSIM_INT_RX_ECC_ERR |
                           DSIM_INT_SW_RST_RELEASE);
    samsung_dsim_write(dsi, DSIM_INTMSK_REG, mask);

Since DSIM_INT_PLL_STABLE is omitted from this unmasking list, its bit
remains 1 due to the bitwise NOT, which actively masks (disables) it in the
hardware.

Because the interrupt is permanently masked in DSIM_INTMSK_REG, will the wait
for completion always time out?

> +
> +	if (!(status & (DSIM_INT_RX_DONE | DSIM_INT_SFR_FIFO_EMPTY)))
>  		return IRQ_HANDLED;
>  
>  	if (samsung_dsim_transfer_finish(dsi))

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723-exynos-dsim-fixes-v3-0-0c31ae1dbecc@disroot.org?part=2

[PATCH v3 3/3] drm/exynos: remove simplefb nodes before init

[Kaustabh Chakraborty]

The simple-framebuffer uses the framebuffer region initialized by the
boot loader for display. However, with Linux the DSI and FIMD/DECON
stack is initialized from scratch.

However, both are destined use the same memory region, so both cannot
co-exist. For a seamless display transition during boot, the framebuffer
must be destroyed, which releases the memory region, followed by proper
IP re-initialization. Achieve it using
aperture_remove_all_conflicting_devices(). This is also implemented in
other similar systems such as in Qualcomm's msm_kms driver.

Signed-off-by: Kaustabh Chakraborty <kauschluss@disroot.org>
---
 drivers/gpu/drm/exynos/exynos_drm_drv.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index 9ee30086879f..7657f86a9d19 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -7,6 +7,7 @@
  *	Seung-Woo Kim <sw0312.kim@samsung.com>
  */
 
+#include <linux/aperture.h>
 #include <linux/component.h>
 #include <linux/dma-mapping.h>
 #include <linux/platform_device.h>
@@ -273,6 +274,11 @@ static int exynos_drm_bind(struct device *dev)
 	if (ret)
 		goto err_mode_config_cleanup;
 
+	/* Remove existing devices which own the framebuffer memory. */
+	ret = aperture_remove_all_conflicting_devices(exynos_drm_driver.name);
+	if (ret)
+		goto err_unbind_all;
+
 	ret = drm_vblank_init(drm, drm->mode_config.num_crtc);
 	if (ret)
 		goto err_unbind_all;

-- 
2.54.0

Re: [PATCH v3 3/3] drm/exynos: remove simplefb nodes before init

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Mismatch between commit message intent and code ordering for removing conflicting framebuffers.
--

--- Patch [3]: [PATCH v3 3/3] drm/exynos: remove simplefb nodes before init ---
commit 7166a20a30c24daba54ef2dcd52625f999da3053
Author: Kaustabh Chakraborty <kauschluss@disroot.org>

drm/exynos: remove simplefb nodes before init

The simple-framebuffer uses the framebuffer region initialized by the
boot loader for display. However, with Linux the DSI and FIMD/DECON
stack is initialized from scratch.

> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> index 9ee30086879f9..7657f86a9d19d 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
[ ... ]
> @@ -273,6 +274,11 @@ static int exynos_drm_bind(struct device *dev)
>  	if (ret)
>  		goto err_mode_config_cleanup;
>  
> +	/* Remove existing devices which own the framebuffer memory. */
> +	ret = aperture_remove_all_conflicting_devices(exynos_drm_driver.name);

[Severity: Medium]
Does this code match the sequence described in the commit message?

The commit message explicitly states that "the framebuffer must be destroyed,
which releases the memory region, followed by proper IP re-initialization."

However, looking at exynos_drm_bind(), this call to
aperture_remove_all_conflicting_devices() happens right after
component_bind_all(). Since component callbacks like fimd_bind() reset the
hardware IPs, the IP re-initialization is actually happening before the
conflicting framebuffer device is removed.

Should the framebuffer removal happen before calling component_bind_all()
instead?

> +	if (ret)
> +		goto err_unbind_all;
> +
>  	ret = drm_vblank_init(drm, drm->mode_config.num_crtc);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723-exynos-dsim-fixes-v3-0-0c31ae1dbecc@disroot.org?part=3