[PATCH 0/3] drm: lcdif: FIFO underrun/solid color bug fix

[PATCH 0/3] drm: lcdif: FIFO underrun/solid color bug fix

[Paul Kocialkowski]

This series brings a fix for the FIFO underrun/solid color bug (in the
last commit) and two other changes that may help with reliability.

Paul Kocialkowski (3):
  drm: lcdif: Set undocumented bit to clear FIFO at vsync
  drm: lcdif: Use dedicated set/clr registers for polarity/edge
  drm: lcdif: Wait for vblank before disabling DMA

 drivers/gpu/drm/mxsfb/lcdif_kms.c  | 42 ++++++++++++++++++++++++------
 drivers/gpu/drm/mxsfb/lcdif_regs.h |  1 +
 2 files changed, 35 insertions(+), 8 deletions(-)

-- 
2.53.0

[PATCH 1/3] drm: lcdif: Set undocumented bit to clear FIFO at vsync

[Paul Kocialkowski]

There is an undocumented bit used in the NXP BSP to clear the FIFO
systematically at vsync. In normal operation, the FIFO should already
be empty but it doesn't hurt to add it as an extra safety measure.

Signed-off-by: Paul Kocialkowski <paulk@sys-base.io>
---
 drivers/gpu/drm/mxsfb/lcdif_kms.c  | 3 ++-
 drivers/gpu/drm/mxsfb/lcdif_regs.h | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c b/drivers/gpu/drm/mxsfb/lcdif_kms.c
index ef3250a5c54f..a00c4f6d63f4 100644
--- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
+++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
@@ -338,7 +338,8 @@ static void lcdif_set_mode(struct lcdif_drm_private *lcdif, u32 bus_flags)
 	 * Downstream set it to 256B burst size to improve the memory
 	 * efficiency so set it here too.
 	 */
-	ctrl = CTRLDESCL0_3_P_SIZE(2) | CTRLDESCL0_3_T_SIZE(2) |
+	ctrl = CTRLDESCL0_3_STATE_CLEAR_VSYNC |
+	       CTRLDESCL0_3_P_SIZE(2) | CTRLDESCL0_3_T_SIZE(2) |
 	       CTRLDESCL0_3_PITCH(lcdif->crtc.primary->state->fb->pitches[0]);
 	writel(ctrl, lcdif->base + LCDC_V8_CTRLDESCL0_3);
 }
diff --git a/drivers/gpu/drm/mxsfb/lcdif_regs.h b/drivers/gpu/drm/mxsfb/lcdif_regs.h
index c55dfb236c1d..17882c593d27 100644
--- a/drivers/gpu/drm/mxsfb/lcdif_regs.h
+++ b/drivers/gpu/drm/mxsfb/lcdif_regs.h
@@ -190,6 +190,7 @@
 #define CTRLDESCL0_1_WIDTH(n)		((n) & 0xffff)
 #define CTRLDESCL0_1_WIDTH_MASK		GENMASK(15, 0)
 
+#define CTRLDESCL0_3_STATE_CLEAR_VSYNC	BIT(23)
 #define CTRLDESCL0_3_P_SIZE(n)		(((n) << 20) & CTRLDESCL0_3_P_SIZE_MASK)
 #define CTRLDESCL0_3_P_SIZE_MASK	GENMASK(22, 20)
 #define CTRLDESCL0_3_T_SIZE(n)		(((n) << 16) & CTRLDESCL0_3_T_SIZE_MASK)
-- 
2.53.0

[PATCH 2/3] drm: lcdif: Use dedicated set/clr registers for polarity/edge

[Paul Kocialkowski]

The lcdif v3 hardware comes with dedicated registers to set and clear
polarity bits in the CTRL register. It is unclear if there is a
difference with writing to the CTRL register directly.

Follow the NXP BSP reference by using these registers, in case there is
a subtle difference caused by using them.

Signed-off-by: Paul Kocialkowski <paulk@sys-base.io>
---
 drivers/gpu/drm/mxsfb/lcdif_kms.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c b/drivers/gpu/drm/mxsfb/lcdif_kms.c
index a00c4f6d63f4..1aac354041c7 100644
--- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
+++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
@@ -296,18 +296,27 @@ static void lcdif_set_formats(struct lcdif_drm_private *lcdif,
 static void lcdif_set_mode(struct lcdif_drm_private *lcdif, u32 bus_flags)
 {
 	struct drm_display_mode *m = &lcdif->crtc.state->adjusted_mode;
-	u32 ctrl = 0;
+	u32 ctrl;
 
 	if (m->flags & DRM_MODE_FLAG_NHSYNC)
-		ctrl |= CTRL_INV_HS;
+		writel(CTRL_INV_HS, lcdif->base + LCDC_V8_CTRL + REG_SET);
+	else
+		writel(CTRL_INV_HS, lcdif->base + LCDC_V8_CTRL + REG_CLR);
+
 	if (m->flags & DRM_MODE_FLAG_NVSYNC)
-		ctrl |= CTRL_INV_VS;
+		writel(CTRL_INV_VS, lcdif->base + LCDC_V8_CTRL + REG_SET);
+	else
+		writel(CTRL_INV_VS, lcdif->base + LCDC_V8_CTRL + REG_CLR);
+
 	if (bus_flags & DRM_BUS_FLAG_DE_LOW)
-		ctrl |= CTRL_INV_DE;
-	if (bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
-		ctrl |= CTRL_INV_PXCK;
+		writel(CTRL_INV_DE, lcdif->base + LCDC_V8_CTRL + REG_SET);
+	else
+		writel(CTRL_INV_DE, lcdif->base + LCDC_V8_CTRL + REG_CLR);
 
-	writel(ctrl, lcdif->base + LCDC_V8_CTRL);
+	if (bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
+		writel(CTRL_INV_PXCK, lcdif->base + LCDC_V8_CTRL + REG_SET);
+	else
+		writel(CTRL_INV_PXCK, lcdif->base + LCDC_V8_CTRL + REG_CLR);
 
 	writel(DISP_SIZE_DELTA_Y(m->vdisplay) |
 	       DISP_SIZE_DELTA_X(m->hdisplay),
-- 
2.53.0

[PATCH 3/3] drm: lcdif: Wait for vblank before disabling DMA

[Paul Kocialkowski]

It is necessary to wait for the full frame to finish streaming
through the DMA engine before we can safely disable it by removing
the DISP_PARA_DISP_ON bit. Disabling it in-flight can leave the
hardware confused and unable to resume streaming for the next frame.

This causes the FIFO underrun and empty status bits to be set and
a single solid color to be shown on the display, coming from one of
the pixels of the previous frame. The issue occurs sporadically when
a new mode is set, which triggers the crtc disable and enable paths.

Setting the shadow load bit and waiting for it to be cleared by the
DMA engine allows waiting for completion.

The NXP BSP driver addresses this issue with a hardcoded 25 ms sleep.

Fixes: 9db35bb349a0 ("drm: lcdif: Add support for i.MX8MP LCDIF variant")
Signed-off-by: Paul Kocialkowski <paulk@sys-base.io>
Co-developed-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/mxsfb/lcdif_kms.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c b/drivers/gpu/drm/mxsfb/lcdif_kms.c
index 1aac354041c7..7dce7f48d938 100644
--- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
+++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
@@ -393,6 +393,22 @@ static void lcdif_disable_controller(struct lcdif_drm_private *lcdif)
 	if (ret)
 		drm_err(lcdif->drm, "Failed to disable controller!\n");
 
+	/*
+	 * It is necessary to wait for the full frame to finish streaming
+	 * through the DMA engine before we can safely disable it by removing
+	 * the DISP_PARA_DISP_ON bit. Disabling it in-flight can leave the
+	 * hardware confused and unable to resume streaming for the next frame.
+	 */
+	reg = readl(lcdif->base + LCDC_V8_CTRLDESCL0_5);
+	reg |= CTRLDESCL0_5_SHADOW_LOAD_EN;
+	writel(reg, lcdif->base + LCDC_V8_CTRLDESCL0_5);
+
+	ret = readl_poll_timeout(lcdif->base + LCDC_V8_CTRLDESCL0_5,
+				 reg, !(reg & CTRLDESCL0_5_SHADOW_LOAD_EN),
+				 0, 36000);	/* Wait ~2 frame times max */
+	if (ret)
+		drm_err(lcdif->drm, "Failed to disable controller!\n");
+
 	reg = readl(lcdif->base + LCDC_V8_DISP_PARA);
 	reg &= ~DISP_PARA_DISP_ON;
 	writel(reg, lcdif->base + LCDC_V8_DISP_PARA);
-- 
2.53.0

Re: [PATCH 1/3] drm: lcdif: Set undocumented bit to clear FIFO at vsync

[Lucas Stach]

Am Dienstag, dem 31.03.2026 um 00:46 +0200 schrieb Paul Kocialkowski:
> There is an undocumented bit used in the NXP BSP to clear the FIFO
> systematically at vsync. In normal operation, the FIFO should already
> be empty but it doesn't hurt to add it as an extra safety measure.
> 
> Signed-off-by: Paul Kocialkowski <paulk@sys-base.io>

Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

> ---
>  drivers/gpu/drm/mxsfb/lcdif_kms.c  | 3 ++-
>  drivers/gpu/drm/mxsfb/lcdif_regs.h | 1 +
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> index ef3250a5c54f..a00c4f6d63f4 100644
> --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> @@ -338,7 +338,8 @@ static void lcdif_set_mode(struct lcdif_drm_private *lcdif, u32 bus_flags)
>  	 * Downstream set it to 256B burst size to improve the memory
>  	 * efficiency so set it here too.
>  	 */
> -	ctrl = CTRLDESCL0_3_P_SIZE(2) | CTRLDESCL0_3_T_SIZE(2) |
> +	ctrl = CTRLDESCL0_3_STATE_CLEAR_VSYNC |
> +	       CTRLDESCL0_3_P_SIZE(2) | CTRLDESCL0_3_T_SIZE(2) |
>  	       CTRLDESCL0_3_PITCH(lcdif->crtc.primary->state->fb->pitches[0]);
>  	writel(ctrl, lcdif->base + LCDC_V8_CTRLDESCL0_3);
>  }
> diff --git a/drivers/gpu/drm/mxsfb/lcdif_regs.h b/drivers/gpu/drm/mxsfb/lcdif_regs.h
> index c55dfb236c1d..17882c593d27 100644
> --- a/drivers/gpu/drm/mxsfb/lcdif_regs.h
> +++ b/drivers/gpu/drm/mxsfb/lcdif_regs.h
> @@ -190,6 +190,7 @@
>  #define CTRLDESCL0_1_WIDTH(n)		((n) & 0xffff)
>  #define CTRLDESCL0_1_WIDTH_MASK		GENMASK(15, 0)
>  
> +#define CTRLDESCL0_3_STATE_CLEAR_VSYNC	BIT(23)
>  #define CTRLDESCL0_3_P_SIZE(n)		(((n) << 20) & CTRLDESCL0_3_P_SIZE_MASK)
>  #define CTRLDESCL0_3_P_SIZE_MASK	GENMASK(22, 20)
>  #define CTRLDESCL0_3_T_SIZE(n)		(((n) << 16) & CTRLDESCL0_3_T_SIZE_MASK)

Re: [PATCH 2/3] drm: lcdif: Use dedicated set/clr registers for polarity/edge

[Lucas Stach]

Hi Paul,

Am Dienstag, dem 31.03.2026 um 00:46 +0200 schrieb Paul Kocialkowski:
> The lcdif v3 hardware comes with dedicated registers to set and clear
> polarity bits in the CTRL register. It is unclear if there is a
> difference with writing to the CTRL register directly.
> 
> Follow the NXP BSP reference by using these registers, in case there is
> a subtle difference caused by using them.
> 
I don't really like that patch, as it blows up what is currently a
single register access to three separate ones. If there is no clear
benefit (as in it has been shown to fix any issue), I would prefer this
code to stay as-is.

Regards,
Lucas

> Signed-off-by: Paul Kocialkowski <paulk@sys-base.io>
> ---
>  drivers/gpu/drm/mxsfb/lcdif_kms.c | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> index a00c4f6d63f4..1aac354041c7 100644
> --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> @@ -296,18 +296,27 @@ static void lcdif_set_formats(struct lcdif_drm_private *lcdif,
>  static void lcdif_set_mode(struct lcdif_drm_private *lcdif, u32 bus_flags)
>  {
>  	struct drm_display_mode *m = &lcdif->crtc.state->adjusted_mode;
> -	u32 ctrl = 0;
> +	u32 ctrl;
>  
>  	if (m->flags & DRM_MODE_FLAG_NHSYNC)
> -		ctrl |= CTRL_INV_HS;
> +		writel(CTRL_INV_HS, lcdif->base + LCDC_V8_CTRL + REG_SET);
> +	else
> +		writel(CTRL_INV_HS, lcdif->base + LCDC_V8_CTRL + REG_CLR);
> +
>  	if (m->flags & DRM_MODE_FLAG_NVSYNC)
> -		ctrl |= CTRL_INV_VS;
> +		writel(CTRL_INV_VS, lcdif->base + LCDC_V8_CTRL + REG_SET);
> +	else
> +		writel(CTRL_INV_VS, lcdif->base + LCDC_V8_CTRL + REG_CLR);
> +
>  	if (bus_flags & DRM_BUS_FLAG_DE_LOW)
> -		ctrl |= CTRL_INV_DE;
> -	if (bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
> -		ctrl |= CTRL_INV_PXCK;
> +		writel(CTRL_INV_DE, lcdif->base + LCDC_V8_CTRL + REG_SET);
> +	else
> +		writel(CTRL_INV_DE, lcdif->base + LCDC_V8_CTRL + REG_CLR);
>  
> -	writel(ctrl, lcdif->base + LCDC_V8_CTRL);
> +	if (bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
> +		writel(CTRL_INV_PXCK, lcdif->base + LCDC_V8_CTRL + REG_SET);
> +	else
> +		writel(CTRL_INV_PXCK, lcdif->base + LCDC_V8_CTRL + REG_CLR);
>  
>  	writel(DISP_SIZE_DELTA_Y(m->vdisplay) |
>  	       DISP_SIZE_DELTA_X(m->hdisplay),

Re: [PATCH 3/3] drm: lcdif: Wait for vblank before disabling DMA

[Lucas Stach]

Am Dienstag, dem 31.03.2026 um 00:46 +0200 schrieb Paul Kocialkowski:
> It is necessary to wait for the full frame to finish streaming
> through the DMA engine before we can safely disable it by removing
> the DISP_PARA_DISP_ON bit. Disabling it in-flight can leave the
> hardware confused and unable to resume streaming for the next frame.
> 
> This causes the FIFO underrun and empty status bits to be set and
> a single solid color to be shown on the display, coming from one of
> the pixels of the previous frame. The issue occurs sporadically when
> a new mode is set, which triggers the crtc disable and enable paths.
> 
> Setting the shadow load bit and waiting for it to be cleared by the
> DMA engine allows waiting for completion.
> 
> The NXP BSP driver addresses this issue with a hardcoded 25 ms sleep.
> 
> Fixes: 9db35bb349a0 ("drm: lcdif: Add support for i.MX8MP LCDIF variant")
> Signed-off-by: Paul Kocialkowski <paulk@sys-base.io>
> Co-developed-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/gpu/drm/mxsfb/lcdif_kms.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> index 1aac354041c7..7dce7f48d938 100644
> --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> @@ -393,6 +393,22 @@ static void lcdif_disable_controller(struct lcdif_drm_private *lcdif)
>  	if (ret)
>  		drm_err(lcdif->drm, "Failed to disable controller!\n");
>  
You can drop this no-op poll above...

> +	/*
> +	 * It is necessary to wait for the full frame to finish streaming
> +	 * through the DMA engine before we can safely disable it by removing
> +	 * the DISP_PARA_DISP_ON bit. Disabling it in-flight can leave the
> +	 * hardware confused and unable to resume streaming for the next frame.
> +	 */
> +	reg = readl(lcdif->base + LCDC_V8_CTRLDESCL0_5);
> +	reg |= CTRLDESCL0_5_SHADOW_LOAD_EN;
> +	writel(reg, lcdif->base + LCDC_V8_CTRLDESCL0_5);
> +
.. then setting the shadow load enable bit can be merged with the
access clearing the DMA enable bit.

> +	ret = readl_poll_timeout(lcdif->base + LCDC_V8_CTRLDESCL0_5,
> +				 reg, !(reg & CTRLDESCL0_5_SHADOW_LOAD_EN),
> +				 0, 36000);	/* Wait ~2 frame times max */

I know this is just a copy from the existing poll, but I don't think
the busy looping makes a lot of sense. I guess relaxing the poll by a
100us or even 200us wait between checks wouldn't hurt.

> +	if (ret)
> +		drm_err(lcdif->drm, "Failed to disable controller!\n");
> +
>  	reg = readl(lcdif->base + LCDC_V8_DISP_PARA);
>  	reg &= ~DISP_PARA_DISP_ON;
>  	writel(reg, lcdif->base + LCDC_V8_DISP_PARA);

Re: [PATCH 3/3] drm: lcdif: Wait for vblank before disabling DMA

[Krzysztof Hałasa]

Hi Paul,

Paul Kocialkowski writes:

> It is necessary to wait for the full frame to finish streaming
> through the DMA engine before we can safely disable it by removing
> the DISP_PARA_DISP_ON bit. Disabling it in-flight can leave the
> hardware confused and unable to resume streaming for the next frame.
>
> This causes the FIFO underrun and empty status bits to be set and
> a single solid color to be shown on the display, coming from one of
> the pixels of the previous frame. The issue occurs sporadically when
> a new mode is set, which triggers the crtc disable and enable paths.
>
> Setting the shadow load bit and waiting for it to be cleared by the
> DMA engine allows waiting for completion.
>
> The NXP BSP driver addresses this issue with a hardcoded 25 ms sleep.

...or "addressed" in the previous version :-)

Test results: it works for me: at 1080p60 and 2160p30. I.e. it fixed the
underrun problem. It's interesting how a CRTC shutdown can affect it's
subsequent start following an SW_RESET.

Or... perhaps it has something to do with the clocks? Also see below.
If somehow the DMA engine was "running" with it's clock disabled, it
would result in an underrun, or worse.


BTW apparently something converted your tab characters into spaces.

> --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> @@ -393,6 +393,22 @@ static void lcdif_disable_controller(struct lcdif_drm_private *lcdif)
>         if (ret)
>                 drm_err(lcdif->drm, "Failed to disable controller!\n");
>
> +       /*
> +        * It is necessary to wait for the full frame to finish streaming
> +        * through the DMA engine before we can safely disable it by removing
> +        * the DISP_PARA_DISP_ON bit. Disabling it in-flight can leave the
> +        * hardware confused and unable to resume streaming for the next frame.
> +        */
> +       reg = readl(lcdif->base + LCDC_V8_CTRLDESCL0_5);
> +       reg |= CTRLDESCL0_5_SHADOW_LOAD_EN;
> +       writel(reg, lcdif->base + LCDC_V8_CTRLDESCL0_5);
> +
> +       ret = readl_poll_timeout(lcdif->base + LCDC_V8_CTRLDESCL0_5,
> +                                reg, !(reg & CTRLDESCL0_5_SHADOW_LOAD_EN),
> +                                0, 36000);     /* Wait ~2 frame times max */

I guess this comment is not necessarily correct - at 2160p30 one frame =
ca. 33 ms. Still works, though (I guess anything more than one frame is
enough). I don't know how long a frame on HDMI (or LVDS, MIPI etc.) can
take. 30 FPS on 2160p is common because the i.MX8MP can't display 2160p60.

Also, found an issue. Perhaps unrelated? Powered the board without HDMI
connected. Then connected 1080p60 display. It came in 1024x768, console
64x24 :-)
Run weston. Pressed ctrl-alt-backspace. It deadlocked. Sysrq (serial
console, show blocked state) showed (with *lcdif* in it):
task:systemd-logind  state:D stack:0     pid:253   tgid:253   ppid:1      task_flags:0x400100 flags:0x00000800
Call trace:
 ...
 schedule+0x34/0x118
 rpm_resume+0x188/0x678
 __pm_runtime_resume+0x4c/0x98
 clk_pm_runtime_get.part.0.isra.0+0x1c/0x94
 clk_core_set_rate_nolock+0xd0/0x2fc
 clk_set_rate+0x38/0x158
 lcdif_crtc_atomic_enable+0x74/0x8d0
 drm_atomic_helper_commit_crtc_enable+0xac/0x104
 drm_atomic_helper_commit_tail_rpm+0x68/0xd8
 commit_tail+0xa4/0x1a4
 drm_atomic_helper_commit+0x178/0x1a0
 drm_atomic_commit+0x8c/0xcc
 drm_client_modeset_commit_atomic+0x1f8/0x25c
 drm_client_modeset_commit_locked+0x60/0x17c
 __drm_fb_helper_restore_fbdev_mode_unlocked.part.0+0x2c/0x8c
 drm_fb_helper_set_par+0x5c/0x78
 fb_set_var+0x190/0x35c
 fbcon_blank+0x178/0x24c
 do_unblank_screen+0xa8/0x19c
 vt_ioctl+0x4fc/0x14c0
 tty_ioctl+0x228/0xb88
 __arm64_sys_ioctl+0x90/0xe4
 ...

This is reproducible, though not always.
It seems it locks on some mutex - the shell works until I do 'cat
log.txt' or similar. Now (with std output/error redirection?), weston
doesn't even start.

dmesg doesn't show anything of interest.
weston: 14.0.2
using /dev/dri/card1
DRM: supports atomic modesetting
DRM: supports GBM modifiers
DRM: does not support Atomic async page flip
DRM: supports picture aspect ratio
Loading module '/usr/lib64/libweston-14/gl-renderer.so'
Using rendering device: /dev/dri/renderD128
EGL version: 1.5
EGL vendor: Mesa Project
EGL client APIs: OpenGL OpenGL_ES
...
Registered plugin API 'weston_drm_output_api_v1' of size 40
Registered plugin API 'weston_drm_virtual_output_api_v2' of size 48
Color manager: no-op
  protocol support: no
Output 'HDMI-A-1' attempts EOTF mode SDR and colorimetry mode default.
Output 'HDMI-A-1' using color profile: stock sRGB color profile
Chosen EGL config details: id:  17 rgba: 8 8 8 0 buf: 24 dep:  0 stcl: 0 int: 1-1 type: win vis_id: XRGB8
)
Output HDMI-A-1 (crtc 37) video modes:
1920x1080@60.0, preferred, current, 148.5 MHz

Output 'HDMI-A-1' enabled with head(s) HDMI-A-1
Loading module '/usr/lib64/weston/desktop-shell.so'
launching '/usr/libexec/weston-keyboard'
launching '/usr/libexec/weston-desktop-shell'
Warning: computed repaint delay for output [HDMI-A-1] is abnormal: -69164 msec (happens always)

could not load cursor 'dnd-copy'
could not load cursor 'dnd-copy'
could not load cursor 'dnd-none'
could not load cursor 'dnd-none'

Why all these clk* mutexes? Perhaps something didn't work out as it
should there? clk_set_rate isn't supposed to take much time, is it?

$ grep clk /tmp/minicom.cap -C1
[  728.310054]  __pm_runtime_resume+0x4c/0x98
[  728.310059]  clk_pm_runtime_get.part.0.isra.0+0x1c/0x94
[  728.310065]  clk_core_set_rate_nolock+0xd0/0x2fc
[  728.310071]  clk_set_rate+0x38/0x158
[  728.310076]  lcdif_crtc_atomic_enable+0x74/0x8d0
--
[  728.310210]  mutex_lock+0x48/0x58
[  728.310216]  clk_prepare_lock+0x80/0xc0
[  728.310223]  clk_unprepare+0x28/0x44
[  728.310227]  fsl_samsung_hdmi_phy_suspend+0x24/0x40
--
[  728.310344]  mutex_lock+0x48/0x58
[  728.310350]  clk_prepare_lock+0x80/0xc0
[  728.310359]  clk_unprepare+0x28/0x44
[  728.310364]  etnaviv_gpu_clk_disable.isra.0+0x28/0x80
[  728.310372]  etnaviv_gpu_rpm_suspend+0x78/0x1dc
--
[  728.310494]  mutex_lock+0x48/0x58
[  728.310499]  clk_prepare_lock+0x80/0xc0
[  728.310506]  clk_unprepare+0x28/0x44
[  728.310512]  sdhci_esdhc_runtime_suspend+0x7c/0x198
--
[  728.310627]  mutex_lock+0x48/0x58
[  728.310632]  clk_prepare_lock+0x80/0xc0
[  728.310639]  clk_round_rate+0x38/0x1d8
[  728.310646]  dev_pm_opp_set_rate+0xe4/0x2e0
--
[  728.310760]  mutex_lock+0x48/0x58
[  728.310765]  clk_prepare_lock+0x80/0xc0
[  728.310771]  clk_prepare+0x1c/0x50
[  728.310778]  sdhci_esdhc_runtime_resume+0x34/0x180
--
[  728.311286]  mutex_lock+0x48/0x58
[  728.311292]  clk_prepare_lock+0x80/0xc0
[  728.311298]  clk_prepare+0x1c/0x50
[  728.311303]  sdhci_esdhc_runtime_resume+0x34/0x180

Something fishy here.
--
Krzysztof "Chris" Hałasa

Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa

Re: [PATCH 2/3] drm: lcdif: Use dedicated set/clr registers for polarity/edge

[Paul Kocialkowski]

[-- Attachment #1: Type: text/plain, Size: 3463 bytes --]

Hi Lucas,

Le Tue 31 Mar 26, 11:09, Lucas Stach a écrit :
> Hi Paul,
> 
> Am Dienstag, dem 31.03.2026 um 00:46 +0200 schrieb Paul Kocialkowski:
> > The lcdif v3 hardware comes with dedicated registers to set and clear
> > polarity bits in the CTRL register. It is unclear if there is a
> > difference with writing to the CTRL register directly.
> > 
> > Follow the NXP BSP reference by using these registers, in case there is
> > a subtle difference caused by using them.
> > 
> I don't really like that patch, as it blows up what is currently a
> single register access to three separate ones. If there is no clear
> benefit (as in it has been shown to fix any issue), I would prefer this
> code to stay as-is.

Well I guess the cost of a few writes vs a single one is rather
negligible. I'm rather worried that there might be an undocumented
reason why these registers exist in the first place and why they are
used in the BSP.

But yes this is only speculation and I could not witness any actual
issue. My setup (lcdif3 with hdmi) uses all positive polarities which is
the default state, so not a good way to check.

It would be great if somebody from NXP could confirm whether this is
needed or not. In the meantime I guess we can drop the patch. It'll stay
on the list in case someone has polarity issues later :)

All the best,

Paul

> Regards,
> Lucas
> 
> > Signed-off-by: Paul Kocialkowski <paulk@sys-base.io>
> > ---
> >  drivers/gpu/drm/mxsfb/lcdif_kms.c | 23 ++++++++++++++++-------
> >  1 file changed, 16 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > index a00c4f6d63f4..1aac354041c7 100644
> > --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > @@ -296,18 +296,27 @@ static void lcdif_set_formats(struct lcdif_drm_private *lcdif,
> >  static void lcdif_set_mode(struct lcdif_drm_private *lcdif, u32 bus_flags)
> >  {
> >  	struct drm_display_mode *m = &lcdif->crtc.state->adjusted_mode;
> > -	u32 ctrl = 0;
> > +	u32 ctrl;
> >  
> >  	if (m->flags & DRM_MODE_FLAG_NHSYNC)
> > -		ctrl |= CTRL_INV_HS;
> > +		writel(CTRL_INV_HS, lcdif->base + LCDC_V8_CTRL + REG_SET);
> > +	else
> > +		writel(CTRL_INV_HS, lcdif->base + LCDC_V8_CTRL + REG_CLR);
> > +
> >  	if (m->flags & DRM_MODE_FLAG_NVSYNC)
> > -		ctrl |= CTRL_INV_VS;
> > +		writel(CTRL_INV_VS, lcdif->base + LCDC_V8_CTRL + REG_SET);
> > +	else
> > +		writel(CTRL_INV_VS, lcdif->base + LCDC_V8_CTRL + REG_CLR);
> > +
> >  	if (bus_flags & DRM_BUS_FLAG_DE_LOW)
> > -		ctrl |= CTRL_INV_DE;
> > -	if (bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
> > -		ctrl |= CTRL_INV_PXCK;
> > +		writel(CTRL_INV_DE, lcdif->base + LCDC_V8_CTRL + REG_SET);
> > +	else
> > +		writel(CTRL_INV_DE, lcdif->base + LCDC_V8_CTRL + REG_CLR);
> >  
> > -	writel(ctrl, lcdif->base + LCDC_V8_CTRL);
> > +	if (bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
> > +		writel(CTRL_INV_PXCK, lcdif->base + LCDC_V8_CTRL + REG_SET);
> > +	else
> > +		writel(CTRL_INV_PXCK, lcdif->base + LCDC_V8_CTRL + REG_CLR);
> >  
> >  	writel(DISP_SIZE_DELTA_Y(m->vdisplay) |
> >  	       DISP_SIZE_DELTA_X(m->hdisplay),
> 

-- 
Paul Kocialkowski,

Independent contractor - sys-base - https://www.sys-base.io/
Free software developer - https://www.paulk.fr/

Expert in multimedia, graphics and embedded hardware support with Linux.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/3] drm: lcdif: Use dedicated set/clr registers for polarity/edge

[Lucas Stach]

Am Dienstag, dem 31.03.2026 um 17:17 +0200 schrieb Paul Kocialkowski:
> Hi Lucas,
> 
> Le Tue 31 Mar 26, 11:09, Lucas Stach a écrit :
> > Hi Paul,
> > 
> > Am Dienstag, dem 31.03.2026 um 00:46 +0200 schrieb Paul Kocialkowski:
> > > The lcdif v3 hardware comes with dedicated registers to set and clear
> > > polarity bits in the CTRL register. It is unclear if there is a
> > > difference with writing to the CTRL register directly.
> > > 
> > > Follow the NXP BSP reference by using these registers, in case there is
> > > a subtle difference caused by using them.
> > > 
> > I don't really like that patch, as it blows up what is currently a
> > single register access to three separate ones. If there is no clear
> > benefit (as in it has been shown to fix any issue), I would prefer this
> > code to stay as-is.
> 
> Well I guess the cost of a few writes vs a single one is rather
> negligible.
> 
Yea, a few writes don't really hurt. But I don't think there is a very
good reason to set this register this way, see below.

> I'm rather worried that there might be an undocumented
> reason why these registers exist in the first place and why they are
> used in the BSP.
> 
> But yes this is only speculation and I could not witness any actual
> issue. My setup (lcdif3 with hdmi) uses all positive polarities which is
> the default state, so not a good way to check.
> 
> It would be great if somebody from NXP could confirm whether this is
> needed or not. In the meantime I guess we can drop the patch. It'll stay
> on the list in case someone has polarity issues later :)

The separate clr/set registers are a rather common design feat found on
Freescale/NXP IP blocks from the MXS era. On some of those IP blocks
_all_ registers are presented as a base/clr/set triplet in the
registers space. As far as I can tell they are mostly useful when you
want to set/clear individual bits from a register without having to
remember or executing a readback of the current state.

In cases like the one changed in this patch, where the full register
state is set in one go, directly writing to the base register is the
right thing to do.

Regards,
Lucas

Re: [PATCH 2/3] drm: lcdif: Use dedicated set/clr registers for polarity/edge

[Paul Kocialkowski]

[-- Attachment #1: Type: text/plain, Size: 2792 bytes --]

Hi Lucas,

On Tue 31 Mar 26, 18:04, Lucas Stach wrote:
> Am Dienstag, dem 31.03.2026 um 17:17 +0200 schrieb Paul Kocialkowski:
> > Le Tue 31 Mar 26, 11:09, Lucas Stach a écrit :
> > > Am Dienstag, dem 31.03.2026 um 00:46 +0200 schrieb Paul Kocialkowski:
> > > > The lcdif v3 hardware comes with dedicated registers to set and clear
> > > > polarity bits in the CTRL register. It is unclear if there is a
> > > > difference with writing to the CTRL register directly.
> > > > 
> > > > Follow the NXP BSP reference by using these registers, in case there is
> > > > a subtle difference caused by using them.
> > > > 
> > > I don't really like that patch, as it blows up what is currently a
> > > single register access to three separate ones. If there is no clear
> > > benefit (as in it has been shown to fix any issue), I would prefer this
> > > code to stay as-is.
> > 
> > Well I guess the cost of a few writes vs a single one is rather
> > negligible.
> > 
> Yea, a few writes don't really hurt. But I don't think there is a very
> good reason to set this register this way, see below.
> 
> > I'm rather worried that there might be an undocumented
> > reason why these registers exist in the first place and why they are
> > used in the BSP.
> > 
> > But yes this is only speculation and I could not witness any actual
> > issue. My setup (lcdif3 with hdmi) uses all positive polarities which is
> > the default state, so not a good way to check.
> > 
> > It would be great if somebody from NXP could confirm whether this is
> > needed or not. In the meantime I guess we can drop the patch. It'll stay
> > on the list in case someone has polarity issues later :)
> 
> The separate clr/set registers are a rather common design feat found on
> Freescale/NXP IP blocks from the MXS era. On some of those IP blocks
> _all_ registers are presented as a base/clr/set triplet in the
> registers space.

Ah yes I vaguely remember seeing this with units used in the imx6.

> As far as I can tell they are mostly useful when you
> want to set/clear individual bits from a register without having to
> remember or executing a readback of the current state.
> 
> In cases like the one changed in this patch, where the full register
> state is set in one go, directly writing to the base register is the
> right thing to do.

Okay then if you're confident these registers are just here for
convenience purposes we can continue writing the full register value.

I'll drop this patch in the next iteration.

All the best,

Paul

-- 
Paul Kocialkowski,

Independent contractor - sys-base - https://www.sys-base.io/
Free software developer - https://www.paulk.fr/

Expert in multimedia, graphics and embedded hardware support with Linux.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/3] drm: lcdif: Wait for vblank before disabling DMA

[Paul Kocialkowski]

[-- Attachment #1: Type: text/plain, Size: 9092 bytes --]

Hi Krzysztof,

On Tue 31 Mar 26, 12:11, Krzysztof Hałasa wrote:
> Paul Kocialkowski writes:
> > It is necessary to wait for the full frame to finish streaming
> > through the DMA engine before we can safely disable it by removing
> > the DISP_PARA_DISP_ON bit. Disabling it in-flight can leave the
> > hardware confused and unable to resume streaming for the next frame.
> >
> > This causes the FIFO underrun and empty status bits to be set and
> > a single solid color to be shown on the display, coming from one of
> > the pixels of the previous frame. The issue occurs sporadically when
> > a new mode is set, which triggers the crtc disable and enable paths.
> >
> > Setting the shadow load bit and waiting for it to be cleared by the
> > DMA engine allows waiting for completion.
> >
> > The NXP BSP driver addresses this issue with a hardcoded 25 ms sleep.
> 
> ...or "addressed" in the previous version :-)
> 
> Test results: it works for me: at 1080p60 and 2160p30. I.e. it fixed the
> underrun problem. It's interesting how a CRTC shutdown can affect it's
> subsequent start following an SW_RESET.

Thanks for confirming! Indeed it's a bit surprising. I'm not sure what SW_RESET
does exactly but it may not concern the DMA part. I've tried using it in the
irq handler when the underrun is reported and it didn't do anything at all.

> Or... perhaps it has something to do with the clocks? Also see below.
> If somehow the DMA engine was "running" with it's clock disabled, it
> would result in an underrun, or worse.

Interestingly I tried to keep the clocks always on as an experiment and it
had the opposite effect: the DMA engine would get confused every time after the
first mode set and disable. So for some reason the disabling of the clocks seems
to mitigate the issue rather than aggravate it.

I think the issue probably boils down to a corrupted internal state of the DMA
engine if it is disabled while streaming pixels. This is more or less what the
comment from NXP suggests.

> BTW apparently something converted your tab characters into spaces.

Ah good to know, thanks!

> > --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > @@ -393,6 +393,22 @@ static void lcdif_disable_controller(struct lcdif_drm_private *lcdif)
> >         if (ret)
> >                 drm_err(lcdif->drm, "Failed to disable controller!\n");
> >
> > +       /*
> > +        * It is necessary to wait for the full frame to finish streaming
> > +        * through the DMA engine before we can safely disable it by removing
> > +        * the DISP_PARA_DISP_ON bit. Disabling it in-flight can leave the
> > +        * hardware confused and unable to resume streaming for the next frame.
> > +        */
> > +       reg = readl(lcdif->base + LCDC_V8_CTRLDESCL0_5);
> > +       reg |= CTRLDESCL0_5_SHADOW_LOAD_EN;
> > +       writel(reg, lcdif->base + LCDC_V8_CTRLDESCL0_5);
> > +
> > +       ret = readl_poll_timeout(lcdif->base + LCDC_V8_CTRLDESCL0_5,
> > +                                reg, !(reg & CTRLDESCL0_5_SHADOW_LOAD_EN),
> > +                                0, 36000);     /* Wait ~2 frame times max */
> 
> I guess this comment is not necessarily correct - at 2160p30 one frame =
> ca. 33 ms. Still works, though (I guess anything more than one frame is
> enough). I don't know how long a frame on HDMI (or LVDS, MIPI etc.) can
> take. 30 FPS on 2160p is common because the i.MX8MP can't display 2160p60.

Honestly I think we're good assuming 30 fps (33 ms) is a lower bound.
And the current 36 ms goes even beyond, so I think it's fine.

> Also, found an issue. Perhaps unrelated? Powered the board without HDMI
> connected. Then connected 1080p60 display. It came in 1024x768, console
> 64x24 :-)

That looks more related to a failure to fetch the EDID from the monitor.
1024x768 is the default fallback that is used in this situation.
Maybe check if there is something wrong with the DDC lines from the hdmi
controller, maybe pinmux etc.

> Run weston. Pressed ctrl-alt-backspace. It deadlocked. Sysrq (serial
> console, show blocked state) showed (with *lcdif* in it):
> task:systemd-logind  state:D stack:0     pid:253   tgid:253   ppid:1      task_flags:0x400100 flags:0x00000800
> Call trace:
>  ...
>  schedule+0x34/0x118
>  rpm_resume+0x188/0x678
>  __pm_runtime_resume+0x4c/0x98
>  clk_pm_runtime_get.part.0.isra.0+0x1c/0x94
>  clk_core_set_rate_nolock+0xd0/0x2fc
>  clk_set_rate+0x38/0x158
>  lcdif_crtc_atomic_enable+0x74/0x8d0
>  drm_atomic_helper_commit_crtc_enable+0xac/0x104
>  drm_atomic_helper_commit_tail_rpm+0x68/0xd8
>  commit_tail+0xa4/0x1a4
>  drm_atomic_helper_commit+0x178/0x1a0
>  drm_atomic_commit+0x8c/0xcc
>  drm_client_modeset_commit_atomic+0x1f8/0x25c
>  drm_client_modeset_commit_locked+0x60/0x17c
>  __drm_fb_helper_restore_fbdev_mode_unlocked.part.0+0x2c/0x8c
>  drm_fb_helper_set_par+0x5c/0x78
>  fb_set_var+0x190/0x35c
>  fbcon_blank+0x178/0x24c
>  do_unblank_screen+0xa8/0x19c
>  vt_ioctl+0x4fc/0x14c0
>  tty_ioctl+0x228/0xb88
>  __arm64_sys_ioctl+0x90/0xe4
>  ...
> 
> This is reproducible, though not always.

That seems to be in the enable path and related to runtime pm.
I don't think it is related to my patch at all.

> It seems it locks on some mutex - the shell works until I do 'cat
> log.txt' or similar. Now (with std output/error redirection?), weston
> doesn't even start.
> 
> dmesg doesn't show anything of interest.
> weston: 14.0.2
> using /dev/dri/card1
> DRM: supports atomic modesetting
> DRM: supports GBM modifiers
> DRM: does not support Atomic async page flip
> DRM: supports picture aspect ratio
> Loading module '/usr/lib64/libweston-14/gl-renderer.so'
> Using rendering device: /dev/dri/renderD128
> EGL version: 1.5
> EGL vendor: Mesa Project
> EGL client APIs: OpenGL OpenGL_ES
> ...
> Registered plugin API 'weston_drm_output_api_v1' of size 40
> Registered plugin API 'weston_drm_virtual_output_api_v2' of size 48
> Color manager: no-op
>   protocol support: no
> Output 'HDMI-A-1' attempts EOTF mode SDR and colorimetry mode default.
> Output 'HDMI-A-1' using color profile: stock sRGB color profile
> Chosen EGL config details: id:  17 rgba: 8 8 8 0 buf: 24 dep:  0 stcl: 0 int: 1-1 type: win vis_id: XRGB8
> )
> Output HDMI-A-1 (crtc 37) video modes:
> 1920x1080@60.0, preferred, current, 148.5 MHz
> 
> Output 'HDMI-A-1' enabled with head(s) HDMI-A-1
> Loading module '/usr/lib64/weston/desktop-shell.so'
> launching '/usr/libexec/weston-keyboard'
> launching '/usr/libexec/weston-desktop-shell'
> Warning: computed repaint delay for output [HDMI-A-1] is abnormal: -69164 msec (happens always)
> 
> could not load cursor 'dnd-copy'
> could not load cursor 'dnd-copy'
> could not load cursor 'dnd-none'
> could not load cursor 'dnd-none'
> 
> Why all these clk* mutexes? Perhaps something didn't work out as it
> should there? clk_set_rate isn't supposed to take much time, is it?

Maybe something to do with the clock driver.
It seems pretty much unrelated to the lcdif driver.

All the best,

Paul

> $ grep clk /tmp/minicom.cap -C1
> [  728.310054]  __pm_runtime_resume+0x4c/0x98
> [  728.310059]  clk_pm_runtime_get.part.0.isra.0+0x1c/0x94
> [  728.310065]  clk_core_set_rate_nolock+0xd0/0x2fc
> [  728.310071]  clk_set_rate+0x38/0x158
> [  728.310076]  lcdif_crtc_atomic_enable+0x74/0x8d0
> --
> [  728.310210]  mutex_lock+0x48/0x58
> [  728.310216]  clk_prepare_lock+0x80/0xc0
> [  728.310223]  clk_unprepare+0x28/0x44
> [  728.310227]  fsl_samsung_hdmi_phy_suspend+0x24/0x40
> --
> [  728.310344]  mutex_lock+0x48/0x58
> [  728.310350]  clk_prepare_lock+0x80/0xc0
> [  728.310359]  clk_unprepare+0x28/0x44
> [  728.310364]  etnaviv_gpu_clk_disable.isra.0+0x28/0x80
> [  728.310372]  etnaviv_gpu_rpm_suspend+0x78/0x1dc
> --
> [  728.310494]  mutex_lock+0x48/0x58
> [  728.310499]  clk_prepare_lock+0x80/0xc0
> [  728.310506]  clk_unprepare+0x28/0x44
> [  728.310512]  sdhci_esdhc_runtime_suspend+0x7c/0x198
> --
> [  728.310627]  mutex_lock+0x48/0x58
> [  728.310632]  clk_prepare_lock+0x80/0xc0
> [  728.310639]  clk_round_rate+0x38/0x1d8
> [  728.310646]  dev_pm_opp_set_rate+0xe4/0x2e0
> --
> [  728.310760]  mutex_lock+0x48/0x58
> [  728.310765]  clk_prepare_lock+0x80/0xc0
> [  728.310771]  clk_prepare+0x1c/0x50
> [  728.310778]  sdhci_esdhc_runtime_resume+0x34/0x180
> --
> [  728.311286]  mutex_lock+0x48/0x58
> [  728.311292]  clk_prepare_lock+0x80/0xc0
> [  728.311298]  clk_prepare+0x1c/0x50
> [  728.311303]  sdhci_esdhc_runtime_resume+0x34/0x180
> 
> Something fishy here.
> --
> Krzysztof "Chris" Hałasa
> 
> Sieć Badawcza Łukasiewicz
> Przemysłowy Instytut Automatyki i Pomiarów PIAP
> Al. Jerozolimskie 202, 02-486 Warszawa

-- 
Paul Kocialkowski,

Independent contractor - sys-base - https://www.sys-base.io/
Free software developer - https://www.paulk.fr/

Expert in multimedia, graphics and embedded hardware support with Linux.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/3] drm: lcdif: Wait for vblank before disabling DMA

[Paul Kocialkowski]

[-- Attachment #1: Type: text/plain, Size: 3411 bytes --]

Hi Lucas,

On Tue 31 Mar 26, 11:14, Lucas Stach wrote:
> Am Dienstag, dem 31.03.2026 um 00:46 +0200 schrieb Paul Kocialkowski:
> > It is necessary to wait for the full frame to finish streaming
> > through the DMA engine before we can safely disable it by removing
> > the DISP_PARA_DISP_ON bit. Disabling it in-flight can leave the
> > hardware confused and unable to resume streaming for the next frame.
> > 
> > This causes the FIFO underrun and empty status bits to be set and
> > a single solid color to be shown on the display, coming from one of
> > the pixels of the previous frame. The issue occurs sporadically when
> > a new mode is set, which triggers the crtc disable and enable paths.
> > 
> > Setting the shadow load bit and waiting for it to be cleared by the
> > DMA engine allows waiting for completion.
> > 
> > The NXP BSP driver addresses this issue with a hardcoded 25 ms sleep.
> > 
> > Fixes: 9db35bb349a0 ("drm: lcdif: Add support for i.MX8MP LCDIF variant")
> > Signed-off-by: Paul Kocialkowski <paulk@sys-base.io>
> > Co-developed-by: Lucas Stach <l.stach@pengutronix.de>
> > ---
> >  drivers/gpu/drm/mxsfb/lcdif_kms.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > index 1aac354041c7..7dce7f48d938 100644
> > --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > @@ -393,6 +393,22 @@ static void lcdif_disable_controller(struct lcdif_drm_private *lcdif)
> >  	if (ret)
> >  		drm_err(lcdif->drm, "Failed to disable controller!\n");
> >  
> You can drop this no-op poll above...

You're right, it looks a bit weird to keep it as it's not needed.

> > +	/*
> > +	 * It is necessary to wait for the full frame to finish streaming
> > +	 * through the DMA engine before we can safely disable it by removing
> > +	 * the DISP_PARA_DISP_ON bit. Disabling it in-flight can leave the
> > +	 * hardware confused and unable to resume streaming for the next frame.
> > +	 */
> > +	reg = readl(lcdif->base + LCDC_V8_CTRLDESCL0_5);
> > +	reg |= CTRLDESCL0_5_SHADOW_LOAD_EN;
> > +	writel(reg, lcdif->base + LCDC_V8_CTRLDESCL0_5);
> > +
> .. then setting the shadow load enable bit can be merged with the
> access clearing the DMA enable bit.

I've just tried it out and it works just as well!

> > +	ret = readl_poll_timeout(lcdif->base + LCDC_V8_CTRLDESCL0_5,
> > +				 reg, !(reg & CTRLDESCL0_5_SHADOW_LOAD_EN),
> > +				 0, 36000);	/* Wait ~2 frame times max */
> 
> I know this is just a copy from the existing poll, but I don't think
> the busy looping makes a lot of sense. I guess relaxing the poll by a
> 100us or even 200us wait between checks wouldn't hurt.

Yeah good point too. I think 200 us is definitely fine since we're looking
at an average wait of a few ms.

Will respin the series then!

Thanks for the review,

Paul

> > +	if (ret)
> > +		drm_err(lcdif->drm, "Failed to disable controller!\n");
> > +
> >  	reg = readl(lcdif->base + LCDC_V8_DISP_PARA);
> >  	reg &= ~DISP_PARA_DISP_ON;
> >  	writel(reg, lcdif->base + LCDC_V8_DISP_PARA);
> 

-- 
Paul Kocialkowski,

Independent contractor - sys-base - https://www.sys-base.io/
Free software developer - https://www.paulk.fr/

Expert in multimedia, graphics and embedded hardware support with Linux.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/3] drm: lcdif: Wait for vblank before disabling DMA

[Krzysztof Hałasa]

Hi Paul,

Paul Kocialkowski <paulk@sys-base.io> writes:

> Interestingly I tried to keep the clocks always on as an experiment and it
> had the opposite effect: the DMA engine would get confused every time after the
> first mode set and disable. So for some reason the disabling of the clocks seems
> to mitigate the issue rather than aggravate it.

Interesting. Fortunately we have a workaround.

>> > +       ret = readl_poll_timeout(lcdif->base + LCDC_V8_CTRLDESCL0_5,
>> > +                                reg, !(reg & CTRLDESCL0_5_SHADOW_LOAD_EN),
>> > +                                0, 36000);     /* Wait ~2 frame times max */
>> 
>> I guess this comment is not necessarily correct - at 2160p30 one frame =
>> ca. 33 ms. Still works, though (I guess anything more than one frame is
>> enough). I don't know how long a frame on HDMI (or LVDS, MIPI etc.) can
>> take. 30 FPS on 2160p is common because the i.MX8MP can't display 2160p60.
>
> Honestly I think we're good assuming 30 fps (33 ms) is a lower bound.
> And the current 36 ms goes even beyond, so I think it's fine.

Right. It is just the comment in the code which is not exactly true.
I.e., we "wait for at least 1 complete frame time". I guess.
Also, the 25 ms in the patch (commit) message is no longer accurate.

>> Also, found an issue. Perhaps unrelated? Powered the board without HDMI
>> connected. Then connected 1080p60 display. It came in 1024x768, console
>> 64x24 :-)
>
> That looks more related to a failure to fetch the EDID from the monitor.
> 1024x768 is the default fallback that is used in this situation.
> Maybe check if there is something wrong with the DDC lines from the hdmi
> controller, maybe pinmux etc.

No no no, I did that on purpose - the monitor was really disconnected at
the boot time. Only then (but before starting weston) i reconnected it.
But it indeed appears to be a separate issue, a software one - a mutex
deadlock on access to clocks and power management. Both enable and
disable paths interfere there. "I will post a patch when I have a patch
to post" :-)

Thanks,
-- 
Krzysztof "Chris" Hałasa

Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa

Re: [PATCH 3/3] drm: lcdif: Wait for vblank before disabling DMA

[Krzysztof Hałasa]

Krzysztof Hałasa <khalasa@piap.pl> writes:

> Also, the 25 ms in the patch (commit) message is no longer accurate.

Aaah, it's before 7 a.m. here :-)
On the second try I was finally able to parse that sentence. Perhaps.
-- 
Krzysztof "Chris" Hałasa

Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa

Re: [PATCH 3/3] drm: lcdif: Wait for vblank before disabling DMA

[Paul Kocialkowski]

[-- Attachment #1: Type: text/plain, Size: 2661 bytes --]

Hi Krzysztof,

Le Fri 03 Apr 26, 06:36, Krzysztof Hałasa a écrit :
> Paul Kocialkowski <paulk@sys-base.io> writes:
> > Interestingly I tried to keep the clocks always on as an experiment and it
> > had the opposite effect: the DMA engine would get confused every time after the
> > first mode set and disable. So for some reason the disabling of the clocks seems
> > to mitigate the issue rather than aggravate it.
> 
> Interesting. Fortunately we have a workaround.
> 
> >> > +       ret = readl_poll_timeout(lcdif->base + LCDC_V8_CTRLDESCL0_5,
> >> > +                                reg, !(reg & CTRLDESCL0_5_SHADOW_LOAD_EN),
> >> > +                                0, 36000);     /* Wait ~2 frame times max */
> >> 
> >> I guess this comment is not necessarily correct - at 2160p30 one frame =
> >> ca. 33 ms. Still works, though (I guess anything more than one frame is
> >> enough). I don't know how long a frame on HDMI (or LVDS, MIPI etc.) can
> >> take. 30 FPS on 2160p is common because the i.MX8MP can't display 2160p60.
> >
> > Honestly I think we're good assuming 30 fps (33 ms) is a lower bound.
> > And the current 36 ms goes even beyond, so I think it's fine.
> 
> Right. It is just the comment in the code which is not exactly true.
> I.e., we "wait for at least 1 complete frame time". I guess.
> Also, the 25 ms in the patch (commit) message is no longer accurate.

In the end I made it 50 ms, which should be fine for all modes, and
adapted the comment in v2.

> >> Also, found an issue. Perhaps unrelated? Powered the board without HDMI
> >> connected. Then connected 1080p60 display. It came in 1024x768, console
> >> 64x24 :-)
> >
> > That looks more related to a failure to fetch the EDID from the monitor.
> > 1024x768 is the default fallback that is used in this situation.
> > Maybe check if there is something wrong with the DDC lines from the hdmi
> > controller, maybe pinmux etc.
> 
> No no no, I did that on purpose - the monitor was really disconnected at
> the boot time. Only then (but before starting weston) i reconnected it.
> But it indeed appears to be a separate issue, a software one - a mutex
> deadlock on access to clocks and power management. Both enable and
> disable paths interfere there. "I will post a patch when I have a patch
> to post" :-)

Okay good to know, I'll get back to you if I also see this in the future :)

All the best,

Paul

-- 
Paul Kocialkowski,

Independent contractor - sys-base - https://www.sys-base.io/
Free software developer - https://www.paulk.fr/

Expert in multimedia, graphics and embedded hardware support with Linux.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]